Re: Log4j Project Guidelines
So it's a good idea to set up your IDE to warn if you override a synchronized method with an unsynchronized method? I remember seeing that one somewhere. On 30 April 2014 00:35, Remko Popma remko.po...@gmail.com wrote: Agreed that unless the synchronization is part of the public API it is better to lock on an internal object (e.g. private Object lock = new Object(); ). On the other hand, OutputStreamManager and subclasses have some synchronized public/protected methods and in this case the synchronization on this is part of the public API. On Wed, Apr 30, 2014 at 1:24 PM, Matt Sicker boa...@gmail.com wrote: If you want more than one condition monitor, it's also better to use Lock. I'll definitely agree on the simplicity of synchronizing on an internal Object instance, though. Locking on this, however, is generally a bad idea because any other code can synchronize on the object as well by accident and cause weird problems. On 29 April 2014 20:51, Gary Gregory garydgreg...@gmail.com wrote: I like the KISS approach for now as well. Gary On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma remko.po...@gmail.comwrote: This should be a separate thread, but anyway... I would oppose using fair locks. Not only will throughput be much less, the benefits are debatable, since fairness of locks does not guarantee fairness of thread scheduling. So we would always pay a price in throughput without any guarantee of upside on the latency variance. Not a good trade-off. The solution here is not using different locking mechanisms but not using locking at all. That is, anyone who is concerned about latency variance (of which starvation is an extreme case) should be using asynchronous loggers. The numbers show that with the disruptor (Async Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means 1000x - 100,000x less latency.) And this is compared to AsyncAppender. Synchronous logging is not even in the picture here. I do agree that synchronizing on a String should be avoided, because Strings can be intern()-ed (and in the ConfigurationFactory case it was a String literal which is intern()ed by default), which means that our lock object is globally visible and other parts of the system could potentially synchronize on it and cause deadlock. That was a nice catch. However, I'm not convinced that java.util.concurrent.Lock is always better than the synchronized keyword. It is a more powerful API, and gives more low-level control, but it comes with more complexity and responsibility, the most obvious one being you need to manually unlock() it. With the synchronized keyword I don't need to worry if I or anyone refactoring the code still correctly unlock()s the lock in a finally clause. It just works. Nice and simple. It is only when I really need the more powerful API that I consider using Locks. For example when using separate read and write locks. Or if I need to use tryLock(). Things that I can't do with the synchronized keyword. Otherwise I prefer to keep it simple. My 2 cents. Remko On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker boa...@gmail.com wrote: Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.com wrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph
Re: Log4j Project Guidelines
I'm pretty sure Eclipse does that. Gary On Wed, Apr 30, 2014 at 9:50 AM, Matt Sicker boa...@gmail.com wrote: So it's a good idea to set up your IDE to warn if you override a synchronized method with an unsynchronized method? I remember seeing that one somewhere. On 30 April 2014 00:35, Remko Popma remko.po...@gmail.com wrote: Agreed that unless the synchronization is part of the public API it is better to lock on an internal object (e.g. private Object lock = new Object(); ). On the other hand, OutputStreamManager and subclasses have some synchronized public/protected methods and in this case the synchronization on this is part of the public API. On Wed, Apr 30, 2014 at 1:24 PM, Matt Sicker boa...@gmail.com wrote: If you want more than one condition monitor, it's also better to use Lock. I'll definitely agree on the simplicity of synchronizing on an internal Object instance, though. Locking on this, however, is generally a bad idea because any other code can synchronize on the object as well by accident and cause weird problems. On 29 April 2014 20:51, Gary Gregory garydgreg...@gmail.com wrote: I like the KISS approach for now as well. Gary On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma remko.po...@gmail.comwrote: This should be a separate thread, but anyway... I would oppose using fair locks. Not only will throughput be much less, the benefits are debatable, since fairness of locks does not guarantee fairness of thread scheduling. So we would always pay a price in throughput without any guarantee of upside on the latency variance. Not a good trade-off. The solution here is not using different locking mechanisms but not using locking at all. That is, anyone who is concerned about latency variance (of which starvation is an extreme case) should be using asynchronous loggers. The numbers show that with the disruptor (Async Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means 1000x - 100,000x less latency.) And this is compared to AsyncAppender. Synchronous logging is not even in the picture here. I do agree that synchronizing on a String should be avoided, because Strings can be intern()-ed (and in the ConfigurationFactory case it was a String literal which is intern()ed by default), which means that our lock object is globally visible and other parts of the system could potentially synchronize on it and cause deadlock. That was a nice catch. However, I'm not convinced that java.util.concurrent.Lock is always better than the synchronized keyword. It is a more powerful API, and gives more low-level control, but it comes with more complexity and responsibility, the most obvious one being you need to manually unlock() it. With the synchronized keyword I don't need to worry if I or anyone refactoring the code still correctly unlock()s the lock in a finally clause. It just works. Nice and simple. It is only when I really need the more powerful API that I consider using Locks. For example when using separate read and write locks. Or if I need to use tryLock(). Things that I can't do with the synchronized keyword. Otherwise I prefer to keep it simple. My 2 cents. Remko On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker boa...@gmail.com wrote: Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.com wrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an
Re: Log4j Project Guidelines
Yes, overriding a synchronized method with an unsynchronized method sounds dangerous... Warnings would probably be helpful. On Wed, Apr 30, 2014 at 11:13 PM, Gary Gregory garydgreg...@gmail.comwrote: I'm pretty sure Eclipse does that. Gary On Wed, Apr 30, 2014 at 9:50 AM, Matt Sicker boa...@gmail.com wrote: So it's a good idea to set up your IDE to warn if you override a synchronized method with an unsynchronized method? I remember seeing that one somewhere. On 30 April 2014 00:35, Remko Popma remko.po...@gmail.com wrote: Agreed that unless the synchronization is part of the public API it is better to lock on an internal object (e.g. private Object lock = new Object(); ). On the other hand, OutputStreamManager and subclasses have some synchronized public/protected methods and in this case the synchronization on this is part of the public API. On Wed, Apr 30, 2014 at 1:24 PM, Matt Sicker boa...@gmail.com wrote: If you want more than one condition monitor, it's also better to use Lock. I'll definitely agree on the simplicity of synchronizing on an internal Object instance, though. Locking on this, however, is generally a bad idea because any other code can synchronize on the object as well by accident and cause weird problems. On 29 April 2014 20:51, Gary Gregory garydgreg...@gmail.com wrote: I like the KISS approach for now as well. Gary On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma remko.po...@gmail.comwrote: This should be a separate thread, but anyway... I would oppose using fair locks. Not only will throughput be much less, the benefits are debatable, since fairness of locks does not guarantee fairness of thread scheduling. So we would always pay a price in throughput without any guarantee of upside on the latency variance. Not a good trade-off. The solution here is not using different locking mechanisms but not using locking at all. That is, anyone who is concerned about latency variance (of which starvation is an extreme case) should be using asynchronous loggers. The numbers show that with the disruptor (Async Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means 1000x - 100,000x less latency.) And this is compared to AsyncAppender. Synchronous logging is not even in the picture here. I do agree that synchronizing on a String should be avoided, because Strings can be intern()-ed (and in the ConfigurationFactory case it was a String literal which is intern()ed by default), which means that our lock object is globally visible and other parts of the system could potentially synchronize on it and cause deadlock. That was a nice catch. However, I'm not convinced that java.util.concurrent.Lock is always better than the synchronized keyword. It is a more powerful API, and gives more low-level control, but it comes with more complexity and responsibility, the most obvious one being you need to manually unlock() it. With the synchronized keyword I don't need to worry if I or anyone refactoring the code still correctly unlock()s the lock in a finally clause. It just works. Nice and simple. It is only when I really need the more powerful API that I consider using Locks. For example when using separate read and write locks. Or if I need to use tryLock(). Things that I can't do with the synchronized keyword. Otherwise I prefer to keep it simple. My 2 cents. Remko On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker boa...@gmail.comwrote: Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.comwrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as
Re: Log4j Project Guidelines
Actually, I believe there was a case where I did that because the method in the subclass used java.util.concurrent instead of requiring method synchronization. Ralph On Apr 30, 2014, at 7:19 AM, Remko Popma remko.po...@gmail.com wrote: Yes, overriding a synchronized method with an unsynchronized method sounds dangerous... Warnings would probably be helpful. On Wed, Apr 30, 2014 at 11:13 PM, Gary Gregory garydgreg...@gmail.com wrote: I'm pretty sure Eclipse does that. Gary On Wed, Apr 30, 2014 at 9:50 AM, Matt Sicker boa...@gmail.com wrote: So it's a good idea to set up your IDE to warn if you override a synchronized method with an unsynchronized method? I remember seeing that one somewhere. On 30 April 2014 00:35, Remko Popma remko.po...@gmail.com wrote: Agreed that unless the synchronization is part of the public API it is better to lock on an internal object (e.g. private Object lock = new Object(); ). On the other hand, OutputStreamManager and subclasses have some synchronized public/protected methods and in this case the synchronization on this is part of the public API. On Wed, Apr 30, 2014 at 1:24 PM, Matt Sicker boa...@gmail.com wrote: If you want more than one condition monitor, it's also better to use Lock. I'll definitely agree on the simplicity of synchronizing on an internal Object instance, though. Locking on this, however, is generally a bad idea because any other code can synchronize on the object as well by accident and cause weird problems. On 29 April 2014 20:51, Gary Gregory garydgreg...@gmail.com wrote: I like the KISS approach for now as well. Gary On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma remko.po...@gmail.com wrote: This should be a separate thread, but anyway... I would oppose using fair locks. Not only will throughput be much less, the benefits are debatable, since fairness of locks does not guarantee fairness of thread scheduling. So we would always pay a price in throughput without any guarantee of upside on the latency variance. Not a good trade-off. The solution here is not using different locking mechanisms but not using locking at all. That is, anyone who is concerned about latency variance (of which starvation is an extreme case) should be using asynchronous loggers. The numbers show that with the disruptor (Async Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means 1000x - 100,000x less latency.) And this is compared to AsyncAppender. Synchronous logging is not even in the picture here. I do agree that synchronizing on a String should be avoided, because Strings can be intern()-ed (and in the ConfigurationFactory case it was a String literal which is intern()ed by default), which means that our lock object is globally visible and other parts of the system could potentially synchronize on it and cause deadlock. That was a nice catch. However, I'm not convinced that java.util.concurrent.Lock is always better than the synchronized keyword. It is a more powerful API, and gives more low-level control, but it comes with more complexity and responsibility, the most obvious one being you need to manually unlock() it. With the synchronized keyword I don't need to worry if I or anyone refactoring the code still correctly unlock()s the lock in a finally clause. It just works. Nice and simple. It is only when I really need the more powerful API that I consider using Locks. For example when using separate read and write locks. Or if I need to use tryLock(). Things that I can't do with the synchronized keyword. Otherwise I prefer to keep it simple. My 2 cents. Remko On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker boa...@gmail.com wrote: Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.com wrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and
Re: Log4j Project Guidelines
There are legitimate reasons to do that as long as you maintain thread-safety such as the case Ralph mentioned. Though that would probably require full control of the API from top to bottom-ish. On 30 April 2014 10:22, Ralph Goers ralph.go...@dslextreme.com wrote: Actually, I believe there was a case where I did that because the method in the subclass used java.util.concurrent instead of requiring method synchronization. Ralph On Apr 30, 2014, at 7:19 AM, Remko Popma remko.po...@gmail.com wrote: Yes, overriding a synchronized method with an unsynchronized method sounds dangerous... Warnings would probably be helpful. On Wed, Apr 30, 2014 at 11:13 PM, Gary Gregory garydgreg...@gmail.comwrote: I'm pretty sure Eclipse does that. Gary On Wed, Apr 30, 2014 at 9:50 AM, Matt Sicker boa...@gmail.com wrote: So it's a good idea to set up your IDE to warn if you override a synchronized method with an unsynchronized method? I remember seeing that one somewhere. On 30 April 2014 00:35, Remko Popma remko.po...@gmail.com wrote: Agreed that unless the synchronization is part of the public API it is better to lock on an internal object (e.g. private Object lock = new Object(); ). On the other hand, OutputStreamManager and subclasses have some synchronized public/protected methods and in this case the synchronization on this is part of the public API. On Wed, Apr 30, 2014 at 1:24 PM, Matt Sicker boa...@gmail.com wrote: If you want more than one condition monitor, it's also better to use Lock. I'll definitely agree on the simplicity of synchronizing on an internal Object instance, though. Locking on this, however, is generally a bad idea because any other code can synchronize on the object as well by accident and cause weird problems. On 29 April 2014 20:51, Gary Gregory garydgreg...@gmail.com wrote: I like the KISS approach for now as well. Gary On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma remko.po...@gmail.comwrote: This should be a separate thread, but anyway... I would oppose using fair locks. Not only will throughput be much less, the benefits are debatable, since fairness of locks does not guarantee fairness of thread scheduling. So we would always pay a price in throughput without any guarantee of upside on the latency variance. Not a good trade-off. The solution here is not using different locking mechanisms but not using locking at all. That is, anyone who is concerned about latency variance (of which starvation is an extreme case) should be using asynchronous loggers. The numbers show that with the disruptor (Async Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means 1000x - 100,000x less latency.) And this is compared to AsyncAppender. Synchronous logging is not even in the picture here. I do agree that synchronizing on a String should be avoided, because Strings can be intern()-ed (and in the ConfigurationFactory case it was a String literal which is intern()ed by default), which means that our lock object is globally visible and other parts of the system could potentially synchronize on it and cause deadlock. That was a nice catch. However, I'm not convinced that java.util.concurrent.Lock is always better than the synchronized keyword. It is a more powerful API, and gives more low-level control, but it comes with more complexity and responsibility, the most obvious one being you need to manually unlock() it. With the synchronized keyword I don't need to worry if I or anyone refactoring the code still correctly unlock()s the lock in a finally clause. It just works. Nice and simple. It is only when I really need the more powerful API that I consider using Locks. For example when using separate read and write locks. Or if I need to use tryLock(). Things that I can't do with the synchronized keyword. Otherwise I prefer to keep it simple. My 2 cents. Remko On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker boa...@gmail.comwrote: Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.comwrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote:
Re: Log4j Project Guidelines
Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph Goers wrote: Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock instead. There is little to no value in doing that and makes the code look a little more cluttered. However, if a ReadWriteLock can be used in place of synchronized that is a real benefit. The point of the guidelines are that when it comes to stuff like this, unless there is a guideline written down that says the current code is wrong discuss it on the list before making a change. Ralph On Apr 28, 2014, at 9:35 AM, Matt Sicker wrote: What about style things covered by Effective Java? These are pretty much all good ideas. Also, how about some guidelines on concurrency? I'd recommend not using the old concurrency stuff and instead using java.util.concurrent.* for more effective concurrency. This doesn't include the Thread vs Runnable thing, so that's still debatable. On 28 April 2014 08:46, Gary Gregory wrote: Perhaps if one breaks the build, it should be polite to revert that last commit... Gary On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goerswrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org Java Persistence with Hibernate, Second Edition JUnit in Action, Second Edition Spring Batch in Action Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory -- Matt Sicker
Re: Log4j Project Guidelines
Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.com wrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph Goers wrote: Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock instead. There is little to no value in doing that and makes the code look a little more cluttered. However, if a ReadWriteLock can be used in place of synchronized that is a real benefit. The point of the guidelines are that when it comes to stuff like this, unless there is a guideline written down that says the current code is wrong discuss it on the list before making a change. Ralph On Apr 28, 2014, at 9:35 AM, Matt Sicker wrote: What about style things covered by Effective Java? These are pretty much all good ideas. Also, how about some guidelines on concurrency? I'd recommend not using the old concurrency stuff and instead using java.util.concurrent.* for more effective concurrency. This doesn't include the Thread vs Runnable thing, so that's still debatable. On 28 April 2014 08:46, Gary Gregory wrote: Perhaps if one breaks the build, it should be polite to revert that last commit... Gary On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goerswrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail:
Re: Log4j Project Guidelines
I like the KISS approach for now as well. Gary On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma remko.po...@gmail.com wrote: This should be a separate thread, but anyway... I would oppose using fair locks. Not only will throughput be much less, the benefits are debatable, since fairness of locks does not guarantee fairness of thread scheduling. So we would always pay a price in throughput without any guarantee of upside on the latency variance. Not a good trade-off. The solution here is not using different locking mechanisms but not using locking at all. That is, anyone who is concerned about latency variance (of which starvation is an extreme case) should be using asynchronous loggers. The numbers show that with the disruptor (Async Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means 1000x - 100,000x less latency.) And this is compared to AsyncAppender. Synchronous logging is not even in the picture here. I do agree that synchronizing on a String should be avoided, because Strings can be intern()-ed (and in the ConfigurationFactory case it was a String literal which is intern()ed by default), which means that our lock object is globally visible and other parts of the system could potentially synchronize on it and cause deadlock. That was a nice catch. However, I'm not convinced that java.util.concurrent.Lock is always better than the synchronized keyword. It is a more powerful API, and gives more low-level control, but it comes with more complexity and responsibility, the most obvious one being you need to manually unlock() it. With the synchronized keyword I don't need to worry if I or anyone refactoring the code still correctly unlock()s the lock in a finally clause. It just works. Nice and simple. It is only when I really need the more powerful API that I consider using Locks. For example when using separate read and write locks. Or if I need to use tryLock(). Things that I can't do with the synchronized keyword. Otherwise I prefer to keep it simple. My 2 cents. Remko On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker boa...@gmail.com wrote: Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.com wrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph Goers wrote: Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock instead. There is little to no value in doing that and makes the code look a little more cluttered. However, if a ReadWriteLock can be used in place of synchronized that is a real benefit. The point of the guidelines are that when it comes to stuff like this, unless there is a guideline written down that says the current code is wrong discuss it on the list before making a change. Ralph On Apr 28, 2014, at 9:35 AM, Matt Sicker wrote: What about style things covered by Effective Java? These are pretty much all good ideas. Also, how about some guidelines on concurrency? I'd recommend not using the old concurrency stuff and instead using java.util.concurrent.* for more effective concurrency. This doesn't include the Thread vs Runnable
Re: Log4j Project Guidelines
If you want more than one condition monitor, it's also better to use Lock. I'll definitely agree on the simplicity of synchronizing on an internal Object instance, though. Locking on this, however, is generally a bad idea because any other code can synchronize on the object as well by accident and cause weird problems. On 29 April 2014 20:51, Gary Gregory garydgreg...@gmail.com wrote: I like the KISS approach for now as well. Gary On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma remko.po...@gmail.comwrote: This should be a separate thread, but anyway... I would oppose using fair locks. Not only will throughput be much less, the benefits are debatable, since fairness of locks does not guarantee fairness of thread scheduling. So we would always pay a price in throughput without any guarantee of upside on the latency variance. Not a good trade-off. The solution here is not using different locking mechanisms but not using locking at all. That is, anyone who is concerned about latency variance (of which starvation is an extreme case) should be using asynchronous loggers. The numbers show that with the disruptor (Async Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means 1000x - 100,000x less latency.) And this is compared to AsyncAppender. Synchronous logging is not even in the picture here. I do agree that synchronizing on a String should be avoided, because Strings can be intern()-ed (and in the ConfigurationFactory case it was a String literal which is intern()ed by default), which means that our lock object is globally visible and other parts of the system could potentially synchronize on it and cause deadlock. That was a nice catch. However, I'm not convinced that java.util.concurrent.Lock is always better than the synchronized keyword. It is a more powerful API, and gives more low-level control, but it comes with more complexity and responsibility, the most obvious one being you need to manually unlock() it. With the synchronized keyword I don't need to worry if I or anyone refactoring the code still correctly unlock()s the lock in a finally clause. It just works. Nice and simple. It is only when I really need the more powerful API that I consider using Locks. For example when using separate read and write locks. Or if I need to use tryLock(). Things that I can't do with the synchronized keyword. Otherwise I prefer to keep it simple. My 2 cents. Remko On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker boa...@gmail.com wrote: Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.com wrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph Goers wrote: Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock instead. There is little to no value in doing that and makes the code look a little more cluttered. However, if a ReadWriteLock can be used in place of synchronized that is a real benefit. The point of the guidelines are that when it comes to stuff like this, unless there is a guideline written down that says the current code is wrong discuss it on the list before making a change.
Re: Log4j Project Guidelines
Agreed that unless the synchronization is part of the public API it is better to lock on an internal object (e.g. private Object lock = new Object(); ). On the other hand, OutputStreamManager and subclasses have some synchronized public/protected methods and in this case the synchronization on this is part of the public API. On Wed, Apr 30, 2014 at 1:24 PM, Matt Sicker boa...@gmail.com wrote: If you want more than one condition monitor, it's also better to use Lock. I'll definitely agree on the simplicity of synchronizing on an internal Object instance, though. Locking on this, however, is generally a bad idea because any other code can synchronize on the object as well by accident and cause weird problems. On 29 April 2014 20:51, Gary Gregory garydgreg...@gmail.com wrote: I like the KISS approach for now as well. Gary On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma remko.po...@gmail.comwrote: This should be a separate thread, but anyway... I would oppose using fair locks. Not only will throughput be much less, the benefits are debatable, since fairness of locks does not guarantee fairness of thread scheduling. So we would always pay a price in throughput without any guarantee of upside on the latency variance. Not a good trade-off. The solution here is not using different locking mechanisms but not using locking at all. That is, anyone who is concerned about latency variance (of which starvation is an extreme case) should be using asynchronous loggers. The numbers show that with the disruptor (Async Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means 1000x - 100,000x less latency.) And this is compared to AsyncAppender. Synchronous logging is not even in the picture here. I do agree that synchronizing on a String should be avoided, because Strings can be intern()-ed (and in the ConfigurationFactory case it was a String literal which is intern()ed by default), which means that our lock object is globally visible and other parts of the system could potentially synchronize on it and cause deadlock. That was a nice catch. However, I'm not convinced that java.util.concurrent.Lock is always better than the synchronized keyword. It is a more powerful API, and gives more low-level control, but it comes with more complexity and responsibility, the most obvious one being you need to manually unlock() it. With the synchronized keyword I don't need to worry if I or anyone refactoring the code still correctly unlock()s the lock in a finally clause. It just works. Nice and simple. It is only when I really need the more powerful API that I consider using Locks. For example when using separate read and write locks. Or if I need to use tryLock(). Things that I can't do with the synchronized keyword. Otherwise I prefer to keep it simple. My 2 cents. Remko On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker boa...@gmail.com wrote: Should we be using a fair lock? That's usually a lot slower than a typical one, but if it's more proper behavior, then it would make sense to go that route. On 29 April 2014 14:49, Jörn Huxhorn jhuxh...@googlemail.com wrote: Please keep in mind that synchronized is not fair. http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock Yes, a fair ReentrantLock is way slower than an unfair one… but if starvation is caused by a logging framework then this is a serious issue in my opinion. Joern On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph Goers wrote: Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock
Re: Log4j Project Guidelines
I recommend this gets reposted to the wiki or the site docs. On Mon, Apr 28, 2014 at 2:07 AM, Ralph Goers ralph.go...@dslextreme.comwrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org -- Cheers, Paul
Re: Log4j Project Guidelines
Perhaps if one breaks the build, it should be polite to revert that last commit... Gary On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goers ralph.go...@dslextreme.comwrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org Java Persistence with Hibernate, Second Editionhttp://www.manning.com/bauer3/ JUnit in Action, Second Edition http://www.manning.com/tahchiev/ Spring Batch in Action http://www.manning.com/templier/ Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory
Re: Log4j Project Guidelines
What about style things covered by Effective Java? These are pretty much all good ideas. Also, how about some guidelines on concurrency? I'd recommend not using the old concurrency stuff and instead using java.util.concurrent.* for more effective concurrency. This doesn't include the Thread vs Runnable thing, so that's still debatable. On 28 April 2014 08:46, Gary Gregory garydgreg...@gmail.com wrote: Perhaps if one breaks the build, it should be polite to revert that last commit... Gary On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goers ralph.go...@dslextreme.comwrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org Java Persistence with Hibernate, Second Editionhttp://www.manning.com/bauer3/ JUnit in Action, Second Edition http://www.manning.com/tahchiev/ Spring Batch in Action http://www.manning.com/templier/ Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory -- Matt Sicker boa...@gmail.com
Re: Log4j Project Guidelines
Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock instead. There is little to no value in doing that and makes the code look a little more cluttered. However, if a ReadWriteLock can be used in place of synchronized that is a real benefit. The point of the guidelines are that when it comes to stuff like this, unless there is a guideline written down that says the current code is wrong discuss it on the list before making a change. Ralph On Apr 28, 2014, at 9:35 AM, Matt Sicker boa...@gmail.com wrote: What about style things covered by Effective Java? These are pretty much all good ideas. Also, how about some guidelines on concurrency? I'd recommend not using the old concurrency stuff and instead using java.util.concurrent.* for more effective concurrency. This doesn't include the Thread vs Runnable thing, so that's still debatable. On 28 April 2014 08:46, Gary Gregory garydgreg...@gmail.com wrote: Perhaps if one breaks the build, it should be polite to revert that last commit... Gary On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goers ralph.go...@dslextreme.com wrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org Java Persistence with Hibernate, Second Edition JUnit in Action, Second Edition Spring Batch in Action Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory -- Matt Sicker boa...@gmail.com
Re: Log4j Project Guidelines
In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph Goers ralph.go...@dslextreme.com wrote: Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock instead. There is little to no value in doing that and makes the code look a little more cluttered. However, if a ReadWriteLock can be used in place of synchronized that is a real benefit. The point of the guidelines are that when it comes to stuff like this, unless there is a guideline written down that says the current code is wrong discuss it on the list before making a change. Ralph On Apr 28, 2014, at 9:35 AM, Matt Sicker boa...@gmail.com wrote: What about style things covered by Effective Java? These are pretty much all good ideas. Also, how about some guidelines on concurrency? I'd recommend not using the old concurrency stuff and instead using java.util.concurrent.* for more effective concurrency. This doesn't include the Thread vs Runnable thing, so that's still debatable. On 28 April 2014 08:46, Gary Gregory garydgreg...@gmail.com wrote: Perhaps if one breaks the build, it should be polite to revert that last commit... Gary On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goers ralph.go...@dslextreme.comwrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org Java Persistence with Hibernate, Second Editionhttp://www.manning.com/bauer3/ JUnit in Action, Second Edition http://www.manning.com/tahchiev/ Spring Batch in Action http://www.manning.com/templier/ Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory -- Matt Sicker boa...@gmail.com -- Matt Sicker boa...@gmail.com
Re: Log4j Project Guidelines
Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker boa...@gmail.com wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph Goers ralph.go...@dslextreme.com wrote: Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock instead. There is little to no value in doing that and makes the code look a little more cluttered. However, if a ReadWriteLock can be used in place of synchronized that is a real benefit. The point of the guidelines are that when it comes to stuff like this, unless there is a guideline written down that says the current code is wrong discuss it on the list before making a change. Ralph On Apr 28, 2014, at 9:35 AM, Matt Sicker boa...@gmail.com wrote: What about style things covered by Effective Java? These are pretty much all good ideas. Also, how about some guidelines on concurrency? I'd recommend not using the old concurrency stuff and instead using java.util.concurrent.* for more effective concurrency. This doesn't include the Thread vs Runnable thing, so that's still debatable. On 28 April 2014 08:46, Gary Gregory garydgreg...@gmail.com wrote: Perhaps if one breaks the build, it should be polite to revert that last commit... Gary On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goers ralph.go...@dslextreme.com wrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org Java Persistence with Hibernate, Second Edition JUnit in Action, Second Edition Spring Batch in Action Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory -- Matt Sicker boa...@gmail.com -- Matt Sicker boa...@gmail.com
Re: Log4j Project Guidelines
I'll delegate my arguments to the SO post about it: http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java In short, it's defensive programming. It's safer to synchronize on an internal object than on this. Plus, it aids in concurrency throughput instead of just using a synchronized method. On 28 April 2014 12:45, Ralph Goers ralph.go...@dslextreme.com wrote: Why are they not appropriate lock objects? Start a discussion before just changing them. Ralph On Apr 28, 2014, at 10:40 AM, Matt Sicker boa...@gmail.com wrote: In that case, Item 69: prefer concurrency utilities to wait and notify. Sounds like we can just use a plain Object instance to lock (which is pretty much equivalent to using ReentrantLock) when doing normal locks, but instead of using .notifyAll() and .wait(), we should use the Condition interface (which would require using Lock as well). I agree that using synchronized(object) makes sense when it's all that's being done. However, I've been changing instances of synchronized(this) and synchronized(foo) where foo is not an appropriate lock object (e.g., a string, or a non-final object, things like that). On 28 April 2014 12:28, Ralph Goers ralph.go...@dslextreme.com wrote: Yes, guidelines called out by Effective Java are appropriate when they apply. As for concurrency, “New” isn’t always better than old. In a few places you changed synchronized(object) to use a Lock instead. There is little to no value in doing that and makes the code look a little more cluttered. However, if a ReadWriteLock can be used in place of synchronized that is a real benefit. The point of the guidelines are that when it comes to stuff like this, unless there is a guideline written down that says the current code is wrong discuss it on the list before making a change. Ralph On Apr 28, 2014, at 9:35 AM, Matt Sicker boa...@gmail.com wrote: What about style things covered by Effective Java? These are pretty much all good ideas. Also, how about some guidelines on concurrency? I'd recommend not using the old concurrency stuff and instead using java.util.concurrent.* for more effective concurrency. This doesn't include the Thread vs Runnable thing, so that's still debatable. On 28 April 2014 08:46, Gary Gregory garydgreg...@gmail.com wrote: Perhaps if one breaks the build, it should be polite to revert that last commit... Gary On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goers ralph.go...@dslextreme.com wrote: I think we need to develop and post some development “guidelines”, “best practices” or whatever you want to call it for Log4j 2. Here are some of the things I would definitely want on it. 1. Error on the side of caution. If you don’t understand it, don’t touch it and ask on the list. If you think you understand it read it again or ask until you are sure you do. Nobody will blame you for asking questions. 2. Don’t break the build - if there is the slightest chance the change you are making could cause unit test failures, run all unit tests. Better yet, get in the habit of always running the unit tests before doing the commit. 3. If the build breaks and you have made recent changes then assume you broke it and try to fix it. Although it might not have been something you did it will make others feel a lot better than having to fix the mistake for you. Everyone makes mistakes. Taking responsibility for them is a good thing. 4. Don’t change things to match your personal preference - the project has style guidelines that are validated with checkstyle, PMD, and other tools. If you aren’t fixing a bug, fixing a problem identified by the tools, or fixing something specifically called out in these guidelines then start a discussion to see if the change is something the project wants before starting to work on it. We try to discuss things first and then implement the consensus reached in the discussion. Of course, the actual coding conventions we follow should also be spelled out, such as indentation, braces style, import ordering and where to use the final keyword. Ralph - To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org -- E-Mail: garydgreg...@gmail.com | ggreg...@apache.org Java Persistence with Hibernate, Second Editionhttp://www.manning.com/bauer3/ JUnit in Action, Second Edition http://www.manning.com/tahchiev/ Spring Batch in Action http://www.manning.com/templier/ Blog: http://garygregory.wordpress.com Home: http://garygregory.com/ Tweet! http://twitter.com/GaryGregory -- Matt Sicker boa...@gmail.com -- Matt Sicker boa...@gmail.com -- Matt Sicker boa...@gmail.com
