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.

Sign up for GitHub

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

Jump to bottom

Go back to the previous ABBP algo and fix it #11428

Closed
wants to merge 3 commits into from
Closed

Go back to the previous ABBP algo and fix it #11428

wants to merge 3 commits into from

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Feb 21, 2024 •
edited
Loading

Alternative to #11426

@lorban
Go back to the previous ABBP algo and fix it
eb4d690 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
gregw
gregw reviewed Feb 21, 2024
View reviewed changes
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java
long total = 0L;
for (RetainedBucket bucket : buckets)
{
total += bucket._pool.size() * (long)bucket._capacity;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this number is? It is all idle and inuse buffers from any concurrent pool plus just the idle buffers from any queued pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the known memory consumption, which doesn't mean much.

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java
long maxMemory = direct ? _maxDirectMemory : _maxHeapMemory;
if (maxMemory > 0)
{
long excess = getMemory(direct) - maxMemory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a full iteration over buckets with a full iteration over each concurrent pool entry for every pool miss. This is not different to my PRs always checking memory in the retain method.

However, you do not check max memory for a normal release, and it certainly can be exceeded (especially with the strange impl of getMemory).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worse than your impl, and the memory accounting is quite strange indeed.

@lorban
make benchmark more accurate by raising thread count and making memor…
c2c9332 
…y-limited fast path test actually hit the limit

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban
fix NPE
83015ff 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban
Copy link
Contributor Author

lorban commented Feb 21, 2024 •
edited
Loading

I'm not convinced this implementation is better than #11426 as when it reaches memory capacity, very many acquisitions won't be served and will trigger an eviction, making the performance plummet quite badly.

Here's what the microbenchmark has to say about this:

Benchmark                                                                     (factor)  (maxBucketSize)  (maxCapacity)  (maxMemory)  (minCapacity)  (type)   Mode  Cnt          Score          Error  Units
ArrayByteBufferPoolBenchmark.fastPathAcquireRelease                               4096               -1          65536            0              0    greg  thrpt   10  229142102.162 ± 35233219.736  ops/s
ArrayByteBufferPoolBenchmark.fastPathAcquireRelease                               4096               -1          65536            0              0    ludo  thrpt   10  381324499.460 ±  8712489.462  ops/s
ArrayByteBufferPoolBenchmark.fastPathAcquireRelease                               4096               -1          65536      1048576              0    greg  thrpt   10  215758553.527 ±  8279594.160  ops/s
ArrayByteBufferPoolBenchmark.fastPathAcquireRelease                               4096               -1          65536      1048576              0    ludo  thrpt   10  394587715.082 ±  4493314.070  ops/s
ArrayByteBufferPoolBenchmark.inputFixedCapacityOutputRandomCapacity               4096               -1          65536            0              0    greg  thrpt   10   81966597.179 ±  2779241.727  ops/s
ArrayByteBufferPoolBenchmark.inputFixedCapacityOutputRandomCapacity               4096               -1          65536            0              0    ludo  thrpt   10   78507283.615 ±  8224128.776  ops/s
ArrayByteBufferPoolBenchmark.inputFixedCapacityOutputRandomCapacity               4096               -1          65536      1048576              0    greg  thrpt   10   27888036.667 ±  9597693.594  ops/s
ArrayByteBufferPoolBenchmark.inputFixedCapacityOutputRandomCapacity               4096               -1          65536      1048576              0    ludo  thrpt   10     309518.284 ±    65010.749  ops/s
ArrayByteBufferPoolBenchmark.inputFixedCapacityOutputRandomCapacityMigrating      4096               -1          65536            0              0    greg  thrpt   10   79412953.878 ±  4868374.327  ops/s
ArrayByteBufferPoolBenchmark.inputFixedCapacityOutputRandomCapacityMigrating      4096               -1          65536            0              0    ludo  thrpt   10   84964065.875 ± 11437317.943  ops/s
ArrayByteBufferPoolBenchmark.inputFixedCapacityOutputRandomCapacityMigrating      4096               -1          65536      1048576              0    greg  thrpt   10   25604116.168 ± 40112738.478  ops/s
ArrayByteBufferPoolBenchmark.inputFixedCapacityOutputRandomCapacityMigrating      4096               -1          65536      1048576              0    ludo  thrpt   10   30624600.597 ± 47674548.825  ops/s

@lorban
Copy link
Contributor Author

lorban commented Feb 21, 2024

It was worth doing for comparison, but the alternative in #11426 is better.

@lorban lorban closed this Feb 21, 2024
@lorban lorban deleted the experiment/jetty-12.0.x/ArrayByteBufferPool-lorban branch February 21, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lorban @gregw