On 9/21/14 7:29 PM, Phil Steitz wrote: > On 9/21/14 4:51 PM, Bernd Eckenfels wrote: >> 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. > Could be I am misreading the patched code, but I don't think so. I > think it actually preserves the original behavior - the max is > global. The other stats are not. > >> 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. > I was not able to demonstrate any consistent performance difference > in my tests. The code in trunk was a tiny bit faster on average but > some runs of the patched code were faster than some runs of the > unpatched code. It might be worth just changing the max to be "lock > free" and seeing if that change by itself makes any difference.
I did that and still could not demonstrate any improvement. The report in the ticket does show monitor contention, though, so I am ambivalent on this one. What do others think? Phil > > > Phil >> 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
