Save setting the same state in the Vertx local data duplicated context

[franz1981]

This is part of a bigger problem shown by some benchmarks ie https://github.com/franz1981/quarkus-reactive-beer

Feel free to try it at home, it should be able to collect flamegraphs as well and is not I/O bound nor using a database (that means, how much is relevant what I've found, that's a fair and separate question)

Some evidence that is a worthy change:

In violet you can see the VertxCurrentContext.set calls, which set the same io.quarkus.arc.impl.RequestContext$RequestContextState over and over in the same context.
In my case, it happens a lot more often because it is filtering the elements (12) twice (I have observed ~50 identical VertxCurrentContext.set), while with a simpler one (which mimic what hibernate reactive would do while performing a query), just ~4.
In both cases, avoiding performing CHM::put seems a great idea, given that would benefit card marking as mentioned already at #30024

My concern now is that is now biased to make it pay more when such replacement happen instead, how much is it common? @mkouba ?

FYI the othe remaning low hanging fruit cost of context propagation is smallrye/smallrye-context-propagation#424

NOTE
this PR include some code refactoring in the caller class (performing activation) to remove from the methods the "slow paths" and makes smaller (in term of bytecode) each of the refactored methods: this would increase the chance to get inlined according to the OpenJDK inlining mechanism. It would be a great practice to always do like that, if won't harm the eyes of the readers (it shouldn't but is personal!).

[franz1981]

And now that's the last thing I would like to understand

My concern now is that is now biased to make it pay more when such replacement happen instead, how much is it common?

@mkouba ?

[franz1981]

Just noticed a thing

@mkouba

    @Override
    public void activate(ContextState initialState) {
        if (LOG.isTraceEnabled()) {
            traceActivate(initialState);
        }
        if (initialState == null) {
            currentContext.set(new RequestContextState(new ConcurrentHashMap<>()));
            // Fire an event with qualifier @Initialized(RequestScoped.class) if there are any observers for it
            fireIfNotEmpty(initializedNotifier);
        } else {
            if (initialState instanceof RequestContextState) {
                currentContext.set((RequestContextState) initialState);
            } else {
                throw new IllegalArgumentException("Invalid initial state: " + initialState.getClass().getName());
            }
        }
    }
    

on RequestContext, we end up on purpose to keep on setting it AFAU, and we never remove by it (unless is using the thread local fallback).
So my concern shouldn't be valid: when we set a new RequestContextState we likely end up resetting it over and over?

[mkouba]

My concern now is that is now biased to make it pay more when such replacement happen instead, how much is it common?

I think that it's fine in general.

Note that the work performed by ArcContextProvider (impl. of Context Propagation API for CDI) is mostly needless. Unfortunately, we can't just disable it for every use case. Actually, I did try in Quarkus 2.x and it was not possible - see #28984. It could be possible in Quarkus 3 though.

In most applications the quarkus.arc.context-propagation.enabled=false should be a safe performance boost (unless the SmallRye Context Propagation API is used explicitly).

[franz1981]

In most applications the quarkus.arc.context-propagation.enabled=false should be a safe performance boost (unless the SmallRye Context Propagation API is used explicitly).

That's awesome, do we have any docs on this for our users to explain it? Is it worthy?

[geoand]

Probably best to squash the commits

[mkouba]

In most applications the quarkus.arc.context-propagation.enabled=false should be a safe performance boost (unless the SmallRye Context Propagation API is used explicitly).

That's awesome, do we have any docs on this for our users to explain it? Is it worthy?

It's not documented AFAIK. The problem is that there isn't agreement on how and when to use it 🤷. BTW there's #34123.

[franz1981]

Performance difference is pretty relevant I see; and I've removed smallrye decoration as well; getting another (small) performance boost.
I would still like to let common users (that won't disable that) to see a similar performance boost thanks to this PR and smallrye/smallrye-context-propagation#424

[geoand]

Where are we with this one? Is it ready to be merged?

IIUC, we decided to not drop the ConcurrentHashMap because it could lead to subtle issues and we are not yet in a state where we can say that we explicitly don't support the cases that would break. Is that accurate?

[franz1981]

Where are we with this one? Is it ready to be merged?

I need to rebase it and yep, we are ready to go!

Is that accurate?

Yep, we can talk about the future of such in a separate call, but in the current state avoiding wasteful CHM::put seems the easier thing to do ATM

[geoand]

👌

[geoand]

So unless there are any objections, let's merge this when CI goes green

[quarkus-bot]

Failing Jobs - Building 61bd514

Status	Name	Step	Failures	Logs	Raw logs
✔️	Maven Tests - JDK 11
⌛	Maven Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc1	Build	⚠️ Check →	Logs	Raw logs

[franz1981]

@geoand @Ladicek how this look? Other things I could address on it?

[geoand]

+1 for merging