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

Rehabilitate quadratic pool #12711

Merged

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Jan 14, 2025

As discussed with @sbordet, ArrayByteBufferPool.Quadratic could be useful in certain scenarios so we should un-deprecate it and fix its minor issues.

@lorban lorban requested review from gregw and sbordet January 14, 2025 10:58
@lorban lorban self-assigned this Jan 14, 2025
@lorban lorban force-pushed the experiment/jetty-12.1.x/rehabilitate-quadratic-pool branch from c62eede to 7769a33 Compare January 14, 2025 11:01
@lorban lorban force-pushed the experiment/jetty-12.1.x/rehabilitate-quadratic-pool branch from 7769a33 to c1c1153 Compare January 14, 2025 11:02
@lorban lorban marked this pull request as ready for review January 15, 2025 09:27
Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban requested a review from sbordet January 23, 2025 17:43
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Other than renaming, this looks good.

However, see my thought bubble for how to make quadratic pool play nice with TLS (probably a different PR).

Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban requested a review from sbordet January 24, 2025 10:05
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

I'm not sure about Predefined as it is.

The base class only calls the function bucketIndexFor, so when predefined is asked for a large buffer that has no bucket, it won't call the other function, and just return whatever was asked, unpooled.
Considering this should simplify the functions.

I don't mind the idea of a Default where the sizes are [1K, 2K, 4K, 8K, 16K, 17K, 32K, 64K].

IIRC, we tried the idea of a "learning" one in the past, but it was very difficult to figure out the capacities, and required a lot of extra code to figure them out.

Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban requested a review from sbordet January 28, 2025 14:10
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

It is really good that we now have these alternative options to tune the byte buffer pool algorithm. However, what we don't have is metrics/stats collected in the normal pool that will easily allow us to come up with a configuration of these pools.

Specifically we have bucket.recordAcquire(), when we really need bucket.recordAcquire(size), so we can record statistics on what the actual requested size is. Otherwise we might know that we have lots of 32KB buffers, but we won't know that only 17KBs were requested.

So I think this PR needs to improve the statistics collected and to make them available in the dump, so that a dump on shutdown can provide the information needed to pick and configure the right optimized pool

@sbordet
Copy link
Contributor

sbordet commented Jan 29, 2025

@gregw the default pool has 4K increments in buffer sizes, so it is already granular enough.

I don't think recordAcquire(size) is necessary; if you really want to know, just provide a factor of 1024 instead of 4096 and cover 0 to 1 MiB capacities and you have your granular stats.

The stats will depend so much on HTTP protocol version, WebSocket, TLS, QUIC, etc. that probably what's good in one case is not for another.

My point being that we don't really want recordAcquire(size) to be costly in the hot path by populating a map of sizes to counters.

@gregw
Copy link
Contributor

gregw commented Jan 29, 2025

@sbordet it is precisely because buffer usage will vary greatly that recording detailed stats from a live system is necessary to well optimize the buffers. I don't think many users will be prepared to do experiments with 1K buffers in production systems. 4K is not good enough granularity as there may be 1K and/or 2K buffers requested.

Adding the size to the signature doesn't mean that the stats always have to be kept, it just means that we can have an option to keep them. If they prove to be expensive, that option can be turned off.

This should be a stats on all impls, so that if predefined buffer sizes are wrong, then we know. It could be as simple as counting how often a size is more that X% of the capcity. I don't think that is a huge expense.

Signed-off-by: Ludovic Orban <[email protected]>
@lorban
Copy link
Contributor Author

lorban commented Jan 29, 2025

@sbordet we discussed it yesterday, when I suggested to add a "waste" stat, @gregw is just suggesting to add a requested size stat, which is similar.
@gregw I added that stat, it was easy and cheap enough. And I do agree that it will help tuning the pool.

@lorban lorban requested a review from gregw January 29, 2025 09:20
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

thanks

@lorban lorban merged commit 139439b into jetty-12.1.x Jan 29, 2025
10 checks passed
@lorban lorban deleted the experiment/jetty-12.1.x/rehabilitate-quadratic-pool branch January 29, 2025 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants