Speedup ContextInstances if there are null instances

[franz1981]

This is an alternative to #38737 with no need of cutoff values and always performing fast-path unlocked checks, if possible, and lazy allocation of a ReentrantLock.

In order to reduce the stack-map it has introduced smaller methods to remove/lazy allocate locks which are reused whenever possible

[franz1981]

@Ladicek @mkouba

[franz1981]

@mkouba I've added 92bc2e5 and update the Java code snippet to explain what is doing: it shouldn't be a big deal but it seems more correct to return only the value read, instead of performing an unguarded volatile read, there.

[mkouba]

I've added few comments but it looks really good!

I will try to run our microbenchmarks to see if it makes any difference...

[franz1981]

Thanks @mkouba yep, in case the micro doesn't impact, I suggest to perform one where the context instance is allocated on the fly and have different instance fields while using a parameter to decide how many to compute, and including a remove at the end of the usage life-cycle ; having such benchmark will help capturing the actual possible behaviours for request scoped beans, which seems to be allocated, computed, removed in the hot path

[mkouba]

So I ran the microbenchmarks and the only benchmark that shows significant improvement is the RequestContextActivationBenchmark which merely tests request context activation/deactivation, no bean operations - and it's expected due to lazy init of Lock. The throughtput is doubled!

The ContextProviderBenchmark that tests ArcContextProvider (SmallRye Context Propagation integration) shows a decent improvement too (because it does not do much more than RequestContextActivationBenchmark). So this PR could slightly improve the performance in use cases where SR Context Propagation is used extensively.

However, the RequestContextBenchmark, which activates the context, invokes client proxies of 5 @RequestScoped beans and finally terminates the context, shows no significant change. And I think that it's expected too because this PR does not improve the ContextInstances#computeIfAbsent() method which is critical for this benchmark. @franz1981 is the use case you describe in the previous comment different?

[franz1981]

@mkouba

@franz1981 is the use case you describe in the previous comment different?

Exactly and happy that you already had crafted other benchmarks which capture the use case (which is farly realistic for the request scoped case) of the changes sent here, thanks again for checking!!!!

Probably RequestContextBenchmark, could have been relevant if the benchmark had a parameter to decide how many of these beans are computed eg for a total of 5 beans, just 1(,2,3,4,5) is computed. It will impact the removal as well, given that this pr remove only what is actually computed, too.

Just for reference

this was the call-stack of a compute case with 1/5 beans were computed (hence saving 3 * 4 = 12 atomic volatile ops in the hot path)

[mkouba]

Probably RequestContextBenchmark, could have been relevant if the benchmark had a parameter to decide how many of these beans are computed eg for a total of 5 beans, just 1(,2,3,4,5) is computed. It will impact the removal as well, given that this pr remove only what is actually computed, too.

So I've tried to adapt the RequestContextBenchmark in a way that:

1. The app contains 10 request scoped beans
2. We always test 15 client proxy invocations but in three variants:
   1. 1 of the 10 beans is used (i.e. instantiated in ContextInstances.computeIfAbsent()),
   2. 3 of the 10 beans are used, and
   3. 5 of the 10 beans are used.

And this PR indeed increased the throughput in the first scenario (1/10) by ~ 40%, in the second (3/10) by ~ 15% and the results of the third are more or less the same.

In other words, the more request scoped beans exist in the app and the less beans are actually used in the benchmark the better throughput with this PR. And since it's very likely that only a fraction of all request scoped beans is used in a single request of a "real world" app I think that it's really a great improvement. Good job!

[franz1981]

I'm adding a note here to help ourselves of the future; in order to "fix" the existing impl and this PR in the case where a compute is started after (no need to be concurrent really) it (regardless the remove has found any value to remove, before):

1. compute always have to check for any existing invalidation before starting
2. after compute has happened, in case it has stored any value, it should check again the invalidation status, and if true, should loop till removing null out everything

The last loop is necessary because other(s) concurrent compute can end up "recomputing" the value again, so it's important for them to keep on trying to remove the value till is stable as null.

This mechanism is to prevent @PreDestroy to NOT be called because the remove will read a null volatile instance value, and the compute will set it right after, without cleaning it up.

@mkouba @Ladicek
I know that's crazy but i can easily forget this concurrent stuff, so let me brain dump this here

[franz1981]

Ok @geoand this should be a very good one, once merged. Just need to make sure the test failures are all flaky ones

[franz1981]

https://github.com/franz1981/quarkus/actions/runs/7941041918 seems to show that the failure reported here is real or another flaky test

[geoand]

@mkouba this looks good, WDYT?

[franz1981]

Done @mkouba and @geoand waiting the CI again, although I've just changed a comment and rebased)

[Ladicek]

getAllPresent() will no longer return a consistent snapshot, but that's not guaranteed for ComputingCacheContextInstances either, so should be fine. Nice work!

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9905b94.

✅ 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.