Zero-Garbage ContextInstances::forEach

[franz1981]

This improvement is necessary due to #36626, which has introduced a very good thing ie bytecode-generated context containers, but it's causing an Set allocation in the hot-path, which eventually lead to calling System::identityHashCode as well (which is not known to be super efficient nor fast).

The cost of Arc in plaintext Techempower for org/jboss/resteasy/reactive/server/core/ResteasyReactiveRequestContext.close was at ~2.87% and has become 6.7%, which is ~ twice as costy as it was before the mentioned pr.

The flamegraphs shows clearly the problem, before:

The problem was that, with a single ctx instance, the default capacity of ConcurrentHashMap was an overkill (16), and forEach and clear were forcing to iterate among all map's slots, including null-ones, twice.

#36626 has introduced a big improvement on it, but:

Shows that:

• an hash map (with default capacity) and an iterator are allocated in the hot path
• given that HashSet's are backed by a map using the values as keys, this force the ctx instances to have their own equals/hashCode implementations ore they would rely on System::identityHashCode which is not great

This pr is "fixing" the "symptom" by providing an optimized forEach method given that ContextInstances is a container itself and I see no harm in this, but still:

1. do we really need the getAllPresent method? If is used elsewhere in the hotpath, is probably a good idea to fix there them as well IMO...
2. I have found ApplicationContextInstancesTest: I still need to write a new test for this additional API method, which use a ctx instances with more than 1 instance in
3. the existing generated implementation still cause lock/unlock to happen while reading the instances, which "could" still be an hot-path: I'm not fixing problems which I cannot observe, but calling volatile stores (twice, to enter/exit) in the hot path have some non-negligible performance impact, which could be improved as well. I don't plan to send the fix for it in this PR.

[franz1981]

@mkouba wdyt? suggestions on how to make it right?

[mkouba]

@mkouba wdyt? suggestions on how to make it right?

I like it! I don't think that we need a default implementation though. ContextInstances is not part of the public API.

do we really need the getAllPresent method? If is used elsewhere in the hotpath, is probably a good idea to fix there them as well IMO...

I think that we need it for some use case with context propagation but it shouldn't be used in a hot path.

I have found ApplicationContextInstancesTest: I still need to write a new test for this additional API method, which use a ctx instances with more than 1 instance in

+1

[franz1981]

@mkouba @geoand ready to go! I've removed the default impl and re-used the method in a place where an existing test is using it. We should be fine now!

[github-actions]

🎊 PR Preview b7887aa has been successfully built and deployed to https://quarkus-pr-main-37529-preview.surge.sh/version/main/guides/

[franz1981]

@mkouba @geoand verified via running Techempower plaintext that we've moved from 3.6 M req/sec to 4 M req/sec
and now the cost for org/jboss/resteasy/reactive/server/core/ResteasyReactiveRequestContext.close is now ~2%, and as expected the 3 atomic ops (2 lock/unlock pairs and req ctx invalidation using a compareAndSet) in the hot paths are the last ones to improve

[geoand]

Great!!!

[mkouba]

🌘 This workflow status is outdated as a new workflow run has been triggered.

⚠️ Unable to include the stracktraces as the report was too long. See annotations below for the details. ⚠️ Unable to include the failure links as the report was too long. See annotations below for the details.

Failing Jobs - Building 098d895

Status Name Step Failures Logs Raw logs Build scan
✖ JVM Tests - JDK 17 Build Failures Logs Raw logs 🚧
✖ JVM Tests - JDK 17 Windows Build Failures Logs Raw logs 🚧
✖ JVM Tests - JDK 21 Build Failures Logs Raw logs 🚧

Failures

⚙️ JVM Tests - JDK 17 #

Just out of curiosity - what caused these failures?

[franz1981]

@mkouba
It was due to https://github.com/quarkusio/quarkus/pull/37529/files#diff-3343cebc078d4441d20bca2600f2139c3b986ee734244472d40c52964758428dR44 which wasn't implemented like that but just iterating the values (without checking if present).

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.