Hello Phil, thanks for taking a look. See my further discussion inline.
Am Sun, 21 Sep 2014 10:30:49 -0700 schrieb Phil Steitz <phil.ste...@gmail.com>: > On 9/19/14 10:50 PM, Bernd Eckenfels wrote: > > re https://issues.apache.org/jira/browse/POOL-277 > > > > I would like to commit VFS-277 fix (nonlockstats2.patch). However > > before I do that, I would like to test if it is really a > > perfomrmance improvement. Lucas confirms it helps in his scenario. > > > > I have run the included PerformanceTest, but the results have been > > indifferent. Are those tests reliable? Can somebody with some > > experience re-run them on their machine? (I somewhat feel the need > > to provide a JMH test :) > > I have started running some performance tests using commons > performance (sandbox). I mostly use [performance] to get races or > other bugs to happen, but it does give an idea of gross performance > differences. So far, I have not been able to detect any performance > benefit from the patch. That does not mean that there is no > benefit, especially since I have only 4 cores on the test box. I > have some longer-running tests running now and will report back if I > find anything. The original reporter reported, that it solved his congestion. But I am also not quite sure what caused the congestion in the first place, and - if there is no contention on the locked section anymore - actually this is better or only a result of beeing slow in other places. I do think the statisticd object is rather lass content in real world scenarios involving external resources. > Even if the patch does improve performance, I am -0 on applying as > is, as the implementation appears to change the meaning of > getMaxBorrowWaitTimeMillis. Actually I think the old meaning was totally broken. It first of all implements some expotential average algorithm without the need for it (as it keeps a lot of samples anyway). And secondly it implements it wrong, as it always starts from index 0, which is not necesarily the right (oldest) index. Both - the old and the new version - are a ringbuffer with a varying start. > The current implementation returns the > max since the pool was created, not a rolling window of recent > values, as the patch appears to do (unless I am missing something). > The unit tests for reported JMX stats are pretty weak, so we need to > be careful relying on them to confirm correctness. I think those are two things, the "max" is in both cases from the start, and the rolling window is supposed to be of the last 100(128) values. (as you have written in your second mail). So from this point of view we are I think safe: yes the calculation will return different results, but the new implementation is more predictable. But I agree we should have a look at the performance some more. We might revert the add() optimizations and only keep the "maximum" lock free implementation. It was reported to help (and again I am not sure why). BTW: I am not entirely sure if we need the Atomic(Array) for the history of last values. I wonder if a volatile array might work as well: it is not entirely correct but as it is only used to collect historical data and it is unlikkely it can and will keep a whole array buffered it might be enugh to go with this (especially since the index is an atomic). The current code is not so easy to test, but I guess I will just copy it into some JMH tests to do some performance testings for the various aspects of getters and setters. Gruss Bernd > Phil > > My proposed patch: > > https://issues.apache.org/jira/secure/attachment/12670185/nolockstats2.patch > > > > Gruss > > Bernd --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org