Jetty 10: backport the tracking retainable pool from 12 #12041
Conversation
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
lorban
added
Enhancement
Sponsored
…ry in the Tracking pool to use a retaining variant Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
| @@ -65,7 +65,7 @@ public LogarithmicArrayByteBufferPool(int minCapacity, int maxCapacity, int maxQ | |||
| */ | |||
| public LogarithmicArrayByteBufferPool(int minCapacity, int maxCapacity, int maxQueueLength, long maxHeapMemory, long maxDirectMemory) | |||
| { | |||
| this(minCapacity, maxCapacity, maxQueueLength, maxHeapMemory, maxDirectMemory, maxHeapMemory, maxDirectMemory); | |||
| this(minCapacity, maxCapacity, maxQueueLength, maxHeapMemory, maxDirectMemory, -2, -2); | |||
There was a problem hiding this comment.
Do we need to document this change in defaults?
There was a problem hiding this comment.
I would say yes, the release notes should indicate that a change in our default pool may have an impact on performance.
You've put your finger on the most sensible part of this PR that we should discuss: what do we want to do for 10/11 in the future with potential buffer leaks? Our options are:
- change the default to use the less performant pool that is leak-resistant (what this change does)
- backport the fast and leak resistant retainable pool from 12 (not a trivial job)
- do nothing (which means spending time tracking reported leaks in releases that are in maintenance mode)
I'm in favor of the 1st option with the following justification: this will minimize the maintenance work of 10/11, the drop in perf is minimal enough that it's only going to affect a small fraction of the user base, it's easy to change your config to revert to the max-perf retainable pool, and upgrading to 12 will give you back the lost perf.
@sbordet @gregw @janbartel WDYT?
There was a problem hiding this comment.
I'm in favour of option 3. Let's not change behaviour significantly in a dot release.
So restore the behaviour here but document the configuration changes needed to get the no retained memory behaviour.
| @@ -65,7 +65,7 @@ public LogarithmicArrayByteBufferPool(int minCapacity, int maxCapacity, int maxQ | |||
| */ | |||
| public LogarithmicArrayByteBufferPool(int minCapacity, int maxCapacity, int maxQueueLength, long maxHeapMemory, long maxDirectMemory) | |||
| { | |||
| this(minCapacity, maxCapacity, maxQueueLength, maxHeapMemory, maxDirectMemory, maxHeapMemory, maxDirectMemory); | |||
| this(minCapacity, maxCapacity, maxQueueLength, maxHeapMemory, maxDirectMemory, -2, -2); | |||
There was a problem hiding this comment.
I'm in favour of option 3. Let's not change behaviour significantly in a dot release.
So restore the behaviour here but document the configuration changes needed to get the no retained memory behaviour.
This reverts commit e2f899e.
This reverts commit b317f28.
This reverts commit 21d8903.
This reverts commit 926544c.
This reverts commit 5b9fdd8.
…rectMemory in the Tracking pool to use a retaining variant" This reverts commit ee4b274.
This reverts commit 5f94238.
Signed-off-by: Ludovic Orban <[email protected]>
This reverts commit edf679d.
|
Alright, so we're going with only adding the tracking pool, and not changing the default retainable pool type. |
lorban
changed the title
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Since Jetty 10's retainable pool is not resistant to leaks, we should change the default to a leak-resistant one.its been decided to not do thatAlso backport the retainable buffer tracking pool from 12 as it is invaluable tracking down retained buffer leaks.