Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Part of #1260: write a manually run concurrency test to tease out problem with pool retention #1265

Merged
merged 3 commits into from
Apr 18, 2024

Conversation

cowtowncoder
Copy link
Member

So, RecyclerPoolTest under src/test/java/perf/ runs through 3 pool implementations with N threads (like 100) over some amount of time (30 or 60 seconds), and checks pooled entry count (pool.pooledCount()); occasionally printing size.
Hope is that this will show abnormal behavior seen in production environments.

@cowtowncoder
Copy link
Member Author

cowtowncoder commented Apr 17, 2024

Running locally, with the latest code -- with 100 threads, 60 seconds for others, 120 seconds for lock-free, I get results:

Tests complete! Results:

 * Completed test of 'LockFreePool': max size seen = 22440
 * Completed test of 'ConcurrentDequePool': max size seen = 29
 * Completed test of 'BoundedPool': max size seen = 34

Interestingly enough, LockFreePool seems to hit its maximum within 1 second (!) of start, probably that is when highest contention is when things are warming up. After that time things always seem to settle.

But the other two implementations appear much better behaved. So I am starting to think that maybe default should actually be changed to ConcurrentDequePool as it has low minimum footprint (important for "single use" usage) while staying within expected boundaries.

EDIT: one more test run:

Tests complete! Results:

 * Completed test of 'LockFreePool': max size seen = 30798
 * Completed test of 'ConcurrentDequePool': max size seen = 34
 * Completed test of 'BoundedPool': max size seen = 37

@cowtowncoder cowtowncoder merged commit 3010ad2 into 2.18 Apr 18, 2024
5 checks passed
@cowtowncoder cowtowncoder deleted the tatu/2.18/1260-recycler-pool-test branch April 18, 2024 00:38
cowtowncoder added a commit that referenced this pull request Apr 18, 2024
@mariofusco
Copy link
Contributor

@cowtowncoder nice reproducer! I also gave a try and came to your similar conclusions, even though I made a few attempts and never seen numbers so high for the LockFreePool, but I guess this also depends on the underlying hardware. I also tried to modified that pool replacing the inner 3 loops before giving up on contention and creating a new resource on the acquire or throwing it away forever on release with an infinite loop and this indeed lowered the max size reached by the pool, but at the cost of a slightly lower throughput.

All this considered I agree on your suggestion of deprecating and ultimately removing the LockFreePool and using the ConcurrentDequePool as the default one for next releases.

@pjfanning
Copy link
Member

pjfanning commented Apr 24, 2024

All of these new pools have much worse performance than the thread pool one. I feel strongly that it is much too early to make the default to use the really slow pools.

@cowtowncoder
Copy link
Member Author

At this point there is just one more thing I would like to confirm wrt performance before rollback for 2.18+ (for 2.17.1 it;s done): is the problem

  1. Simply because the overhead of thread-contention for access in massively parallel (test) cases OR
  2. do the pools fail to recycle buffers thereby being slower than thread-local - one

since (1) is something we perhaps cannot solve.

One way to figure out if (2) might occur would be to profile memory allocations: if recycling works as it should, new pool recycler would not do any more allocations than threadlocal-based pool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants