Stabilize secondary_super_cache in server code

[lorban]

Fix the type pollution of the top two contenders on the server reported in #10781.

Removing the cast-to-interface in these two places boosts perf by a couple of %.

[gregw]

Wow! You'll have to explain this one to me sometime

[gregw]

We have similar code in

• RetainableByteBuffer
• ConnectionMetaData
• ServerUpgradeRequestDelegate
• ServerUpgradeResponseDelegate

Of these, maybe only RetainableByteBuffer may have any impact, but we should remove the anti-pattern from our code and avoid casting our wrapped types

[sbordet]

It is a tradeoff between memory (we don't want to store the same reference in N fields, one of each wrapper), and this JDK issue, and so far it has only surfaced for Request, so I would not bother for the others.

[gregw]

Can we at least look at the usage of retainedbytebuffer wrapper, to see if applies at least a little bit

[lorban]

Before generalizing this pattern, there's a 3rd contender on the list reported by the type pollution agent that should be looked at.

I did a quick analysis, and that 3rd one isn't going to be easy, as IMHO its root lies in a design mistake we made. Very briefly, the ExecutionStrategy.Producer should have specified Invocable.Task instead of plain Runnable.

[lorban]

@sbordet FYI I analyzed the layout of Request.Wrapper in multiple conditions and found out that this extra field is costing 8 bytes in all conditions, making the heap consumption grow from 16 to 24 with G1 and Shenandoah and from 24 to 32 with Z because of compressed oops, padding and memory alignment.

Clearly, we're trading off memory for speed here.

[gregw]

Note that it might be better to simply not use the Attributes.Wrapper base class. I do not think there is any situation that Request.Wrapper must be an Attributes.Wrapper. It can just implement the Attributes delegate methods itself. I think that is a small cost to pay for a space saving on every single request wrapper (and there may be multiple wrappers per real request).

[lorban]

I changed Request.Wrapper so it doesn't extend from Attributes.Wrapper anymore. This gives us the best of both worlds perf wise (i.e.: the speed boost and the best memory efficiency) in exchange of the minuscule usability cost of not being an Attributes.Wrapper and not being able to use Attributes.unwrap anymore.

[lorban]

@gregw RetainableByteBuffer.Wrapper isn't a problem, if only because it's only used by ArrayByteBufferPool.Tracking and tests. This is confirmed by the server report I attached in #10781.

[lorban]

@gregw We should definitely talk about this issue, and other vaguely related things I found out while working on it.

[lorban]

I can confirm the perf gain is real. This is worth nearly as much as our last perf improvement effort!

[sbordet]

Just a couple of nits.

[franz1981]

I can confirm the perf gain is real. This is worth nearly as much as our last perf improvement effort!

Just curious; how much @lorban ? :D

[lorban]

@franz1981 from the top of my head, we got about a 2.5% perf boost overall.

[franz1981]

Thanks! So, measurable beyond noise but still small. If you want to use the latest commit of the agent I have added support for a new type of report which, in some cases, has delivered another for free boost i.e. MISS report.
I can explain here or to @sbordet what it is ;)

[lorban]

@franz1981 I'm always interested in good performance stories, so by any means please go ahead if you have the time to explain another discovery of yours.

I can't promise we'll look into it any time soon though, this performance exercise was done because we were looking to close the small gap between Jetty 10/11 and Jetty 12, and we stopped once all 3 versions achieved similar perf.