Extensible RetainableByteBufferPool #6322 #6513

gregw · 2021-07-12T02:55:13Z

Improves on #6332 for #6322

Add a Log2 bucket size imple for ArrayRetainableByteBufferPool

Even if we don't actually add/use the LogBucket class, I think the changes made to make the ArrayRetainableByteBufferPool extensible are good ones to have.

Signed-off-by: Greg Wilkins gregw@webtide.com

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

…Buffer> for encrypted input Signed-off-by: Ludovic Orban <lorban@bitronix.be>

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

…6322-UseRetainableByteBuffer

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

gregw · 2021-07-13T00:42:31Z

@lorban The intention is not to change the existing behaviour. I don't think I have and the CI failure appears to be exactly that.... a CI failure. Will redo the CI tests to see.

But I'm a strong advocate of making this class extensible, even if we don't add the badly named LogBuckets. Unless the class is extensible then we won't ever be able to do any comparisons.
Also I'll include in this making the pool dumpable so that we can see a lot more detail of how the buckets are being used (regardless of allocation). We can then get some dumps from real live servers and do a calculation to see how alternate bucket strategies might affect the pattern we see.

Dumpable Signed-off-by: Greg Wilkins <gregw@webtide.com>

…f github.com:eclipse/jetty.project into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets Signed-off-by: Greg Wilkins <gregw@webtide.com>

Sampled Pool Signed-off-by: Greg Wilkins <gregw@webtide.com>

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets

gregw · 2021-07-14T00:56:10Z

@sbordet @lorban ignoring the LogBucket class itself, I think this PR has some good improvements for:

code clarity with the Bucket sub class
setting defaults (not duplicated in constructors)
dumpability so we can see the distribution in buckets and how buffers are being used
extensibility, we can have a LogBuckets if it proves necessary.

sbordet · 2021-07-15T10:51:49Z

jetty-io/src/main/java/org/eclipse/jetty/io/ArrayRetainableByteBufferPool.java

+                maxBucketSize,
+                maxHeapMemory,
+                maxDirectMemory,
+                TypeUtil::log2NextPowerOf2,


I'm not against the power of 2 idea, but perhaps it should be stripped out to maintain, as you say, dumpability and statistics.

gregw · 2021-07-15T21:29:50Z

@sbordet The problem with stripping it out, is that we then have no way of actually testing that the class is really extensible. There were a lot of little assumptions in the code that depended on linear buckets. This PR fixes them, but if we took out LogBuckets, it would be easy for a change to revert to having such a assumptions. I could move LogBuckets to test??? But I really don't see any harm having it available, as it will allow us to test with it in real world deployments.

jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java

jetty-io/src/main/java/org/eclipse/jetty/io/ArrayRetainableByteBufferPool.java

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

lorban

I quite like the idea behind this change. More visibility and extensibility is always good to have. I just think that the buckets should be sampled instead of the pool.

lorban · 2021-07-19T08:26:24Z

jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBufferPool.java

+    {
+        private final RetainableByteBufferPool _pool;
+        private final SampleStatistic _heap = new SampleStatistic();
+        private final SampleStatistic _direct = new SampleStatistic();


I think those two counters are going to be a bit meaningless at the pool layer. IMHO it would be a lot more informative to move them to the Bucket layer, maybe by creating a DumpableBucket class.

Hmmm good test for extensibility to have SampledBuckets. Will try....

Actaully, thinking about it, we can just run with a small factor and then we will effectively get a histogram of the acquired sizes. So maybe this class is not needed after all.

jetty-io/src/main/java/org/eclipse/jetty/io/ArrayRetainableByteBufferPool.java

sbordet · 2021-07-19T13:15:24Z

jetty-io/src/main/java/org/eclipse/jetty/io/ArrayRetainableByteBufferPool.java

        if (maxCapacity <= 0)
            maxCapacity = 64 * 1024;
-        if ((maxCapacity % _factor) != 0 || _factor >= maxCapacity)
+
+        final int f = factor <= 0 ? 1024 : factor;


Drop final, not necessary.

sbordet · 2021-07-19T13:27:57Z

jetty-io/src/main/java/org/eclipse/jetty/io/ArrayRetainableByteBufferPool.java

+     * uses buckets of buffers that increase in size by a power of
+     * 2 (eg 1k, 2k, 4k, 8k, etc.).
+     */
+    public static class ExponentialPool extends ArrayRetainableByteBufferPool


Not sure we want to make this public, as it binds us to maintain it, etc.

I want to make it public so we can easily try using it in real deployments.
But as there appears to be resistance to this idea, I'll move to test and we can used the sampled one to gather data to see if ExponentialPool is worthwhile or not.

sbordet · 2021-07-19T13:28:41Z

jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBufferPool.java

+     * @see SampleStatistic
+     */
+    @ManagedObject
+    class SampledPool implements RetainableByteBufferPool, Dumpable


This class is implicitly public, which I don't think it's a good idea.

We want at least this one to be public, so we can run real deployments and capture data about the pool usage. You can't simultaneously argue that ExponentialPool is not justified by any data and then prevent anything that allows the data to be collected!

sbordet · 2021-07-19T13:29:50Z

jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBufferPool.java

+        }
+
+        @ManagedAttribute("Heap acquire samples")
+        public SampleStatistic getHeapSample()


Unfortunately SampleStatistic is not manageable, so this JMX exposure results in no data being exposed.
I think we need either more specific getters, or MXBean-ify SampleStatistic which may not be easy.

I would make this class package private in ArrayRBBP for now, and eventually open it up later.

If it is private, then we can't use it to gather any info.
Anyway, I'm going to look at sampling on buckets rather than on acquire.

sbordet · 2021-07-19T13:31:02Z

jetty-io/src/test/java/org/eclipse/jetty/io/ArrayRetainableByteBufferPoolTest.java

+            capacity = capacity * 2;
+        }
+
+        System.err.println(pool.dump());


Remove this?

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw · 2021-07-20T02:13:37Z

@lorban @sbordet I have removed SampledPool as we get a reasonable histogram of acquire size by using the normal linear pool and dumping the bucket sizes.
I have added a bit more info into the bucket dumps, but as the sampling is in a race, it may be of marginal value on a server underload, but will be OK if dumped after a load test.

I have moved the ExponentialPool to be an inner class of the test. So process from here is to release this.... ask some real clients do give us some server dumps; analyse the "histograms"; if an ExponentialPool looks like it will work, then re-release with that class public and test.

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw · 2021-07-21T01:33:49Z

@lorban @sbordet nudge

lorban

Besides the checkstyle failure, LGTM.

lorban · 2021-07-21T08:38:03Z

jetty-io/src/main/java/org/eclipse/jetty/io/ArrayRetainableByteBufferPool.java

+                }
+            }
+
+            return String.format("%s{capacity=%d,inuse=%d(%d%%),used=%d%%}",


I'm not sure the used calculation will report anything meaningful, a more useful metric would be to calculate the mean 'waste' (i.e.: the difference between the requested size and the returned buffer's capacity) but that would require some extra processing.

I think we should leave this as-is and eventually revisit that later on.

Agreed. We are kind of thwarted by the fact that we have no idea if a buffer is in fill or flush mode.
At least with this metric, it is sampling buffers that have a pointer between 0 and capacity, which is at least a hint of partial usage.

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets fix checkstyle Signed-off-by: Greg Wilkins <gregw@webtide.com>

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets fix checkstyle removed useless metric Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw · 2021-07-21T22:58:52Z

@sbordet nudge!!!

gregw · 2021-07-25T04:14:37Z

Closed in favour of #6538

lorban added 30 commits June 10, 2021 16:30

use RetainableByteBuffer

03f4469

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

fix checkstyle

b22c8cb

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

replace toDetailString with straight RetainableByteBuffer.toString

a093959

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

use RetainableByteBuffer for encrypted input buffer

4c5404d

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

add managed attributes

36794d9

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

add a bit more monitoring to ArrayByteBufferPool

9e58a2a

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

modify SslConnectionFactory to add ability to use Pool<RetainableByte…

ea19cae

…Buffer> for encrypted input Signed-off-by: Ludovic Orban <lorban@bitronix.be>

remove unneeded counters

2ed875c

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

first shot at RetainableByteBuffer pool

2e2a90c

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

cleanup acquire/retain mechanism

45a9b59

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

use pool-specific ctor

2ad1c63

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

add optional ability to use RetainableByteBufferPool on the server

8eb2284

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

revert old ByteBufferPool changes

9b2001d

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

fix compilation error

914a932

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

register RetainableByteBufferPool bean in AbstractConnector

d2ead65

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

add RetainableByteBufferPool registration use in websocket

01b86c0

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

add RetainableByteBufferPool registration use in client

dac025c

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

add RetainableByteBufferPool registration use in upgrade

114418e

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

make RetainableByteBufferPool never null

8efd144

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

make RetainableByteBufferPool never null

db9ba6c

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

make H2 connection use RetainableByteBuffer pool

f8314ef

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

make H2 connection use RetainableByteBuffer pool

27a94db

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

add missing header

c437784

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

generalize RetainableByteBuffer

6eef227

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

enable thread-local cache on the pool

2f6000a

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

do not handle null retainableByteBufferPool

1879cc9

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

make http client register RetainableByteBufferPool

5fc4f51

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

fix concurrency

06580bf

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…

e4606a5

…6322-UseRetainableByteBuffer

add & fix javadoc

6085a77

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

gregw added 4 commits July 13, 2021 11:18

Improve #6322 log buckets in RetainableByteBufferPool

42d4a98

Dumpable Signed-off-by: Greg Wilkins <gregw@webtide.com>

Merge branch 'jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets' o…

791403b

…f github.com:eclipse/jetty.project into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets Signed-off-by: Greg Wilkins <gregw@webtide.com>

Improve #6322 log buckets in RetainableByteBufferPool

d607f21

Sampled Pool Signed-off-by: Greg Wilkins <gregw@webtide.com>

Merge remote-tracking branch 'origin/jetty-10.0.x-6322-UseRetainableB…

56449d9

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets

gregw requested a review from lorban July 13, 2021 06:42

gregw marked this pull request as ready for review July 14, 2021 00:52

sbordet requested changes Jul 15, 2021

View reviewed changes

sbordet requested changes Jul 16, 2021

View reviewed changes

jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java Outdated Show resolved Hide resolved

jetty-io/src/main/java/org/eclipse/jetty/io/ArrayRetainableByteBufferPool.java Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/jetty-10.0.x-6322-UseRetainableB…

ed11f6c

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw requested a review from sbordet July 19, 2021 03:05

lorban requested changes Jul 19, 2021

View reviewed changes

sbordet requested changes Jul 19, 2021

View reviewed changes

Merge remote-tracking branch 'origin/jetty-10.0.x-6322-UseRetainableB…

15c825f

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw requested review from sbordet and lorban July 20, 2021 02:10

Merge remote-tracking branch 'origin/jetty-10.0.x-6322-UseRetainableB…

b70b315

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets Updates from review Signed-off-by: Greg Wilkins <gregw@webtide.com>

gregw changed the title ~~Improve #6322 log buckets in RetainableByteBufferPool~~ Extensible RetainableByteBufferPool #6322 Jul 21, 2021

lorban approved these changes Jul 21, 2021

View reviewed changes

gregw added 2 commits July 22, 2021 08:54

Merge remote-tracking branch 'origin/jetty-10.0.x-6322-UseRetainableB…

5e21903

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets fix checkstyle Signed-off-by: Greg Wilkins <gregw@webtide.com>

Merge remote-tracking branch 'origin/jetty-10.0.x-6322-UseRetainableB…

9b10d54

…yteBuffer' into jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets fix checkstyle removed useless metric Signed-off-by: Greg Wilkins <gregw@webtide.com>

Base automatically changed from jetty-10.0.x-6322-UseRetainableByteBuffer to jetty-10.0.x July 24, 2021 09:19

gregw closed this Jul 25, 2021

joakime deleted the jetty-10.0.x-6322-UseRetainableByteBuffer-LogBuckets branch August 2, 2021 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible RetainableByteBufferPool #6322 #6513

Extensible RetainableByteBufferPool #6322 #6513

gregw commented Jul 12, 2021 •

edited

gregw commented Jul 13, 2021

gregw commented Jul 14, 2021

sbordet Jul 15, 2021

gregw commented Jul 15, 2021

lorban left a comment

lorban Jul 19, 2021

gregw Jul 19, 2021

gregw Jul 19, 2021

sbordet Jul 19, 2021

sbordet Jul 19, 2021

gregw Jul 19, 2021

sbordet Jul 19, 2021

gregw Jul 19, 2021

sbordet Jul 19, 2021

gregw Jul 19, 2021

sbordet Jul 19, 2021

gregw commented Jul 20, 2021

gregw commented Jul 21, 2021

lorban left a comment

lorban Jul 21, 2021

gregw Jul 21, 2021

gregw commented Jul 21, 2021

gregw commented Jul 25, 2021

Extensible RetainableByteBufferPool #6322 #6513

Extensible RetainableByteBufferPool #6322 #6513

Conversation

gregw commented Jul 12, 2021 • edited

gregw commented Jul 13, 2021

gregw commented Jul 14, 2021

Choose a reason for hiding this comment

gregw commented Jul 15, 2021

lorban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Jul 20, 2021

gregw commented Jul 21, 2021

lorban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Jul 21, 2021

gregw commented Jul 25, 2021

gregw commented Jul 12, 2021 •

edited