commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bernd Eckenfels <>
Subject Re: [Pool] Performance Tests for Pool-277
Date Sun, 21 Sep 2014 23:51:29 GMT
Hello Phil, thanks for taking a look.

See my further discussion inline.

Am Sun, 21 Sep 2014 10:30:49 -0700
schrieb Phil Steitz <>:

> On 9/19/14 10:50 PM, Bernd Eckenfels wrote:
> > re
> >
> > 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

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.


> Phil
> > My proposed patch:
> >
> >
> > Gruss
> > Bernd

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message