Don't override finalize() and reduce locking in LoadBalancingPool and ReferenceCountedResourceHandler#3874
Conversation
…d don't override Object.finalize()
…ingResourceHolder
| * connection using a linear search over a simple array, than fiddling with PriorityBlockingQueue. This also allows | ||
| * to reduce locking. | ||
| */ | ||
| private final CountingHolder[] connections; |
There was a problem hiding this comment.
this is the only major difference I see , older class used PriorityBlockingQueue instead to have logN get operations. I guess, size of this pool is usually very small so that difference shouldn't matter
There was a problem hiding this comment.
@himanshug we use 8 clients, unlikely anybody else uses more.
The other change is using Cleaner instead of overriding finalize().
|
@leventov looks good, some tests would be nice. |
…Pool. Add tests for ReferenceCountingResourceHolder and MemcacheClientPool
|
@himanshug added tests |
|
@leventov thanks 👍 |
| throw new ISE("Already closed!"); | ||
| } | ||
| if (refCount.getAndIncrement() <= 0) { | ||
| refCount.decrementAndGet(); |
There was a problem hiding this comment.
When two or more threads call increment() on already closed holders, it looks that some threads might pass this checking accidently because above two operations are not atomic.
There was a problem hiding this comment.
Good catch, thanks! Fixed
|
The latest patch looks good to me. |
| @Override | ||
| public void run() | ||
| { | ||
| try (Releaser r = resourceHolder.increment()) { |
There was a problem hiding this comment.
Should there be a start gate here? or else on an overburdened CI system the threads could easily all start in sequence without actually racing anything
There was a problem hiding this comment.
something like a 100ct CountdownLatch?
There was a problem hiding this comment.
@drcrallen yes, I think it would be good. But PR is already merged.
| { | ||
| synchronized (lock) { | ||
| if (refcount <= 0) { | ||
| while (true) { |
5 participants
A follow-up of #3631
As
LoadBalacingPoolwas used only for pooling memcache clients, it is specialized asMemcacheClientPool.