Micro allocations optimisation in ArC

[Sanne]

I've omitted implementing some of the methods in the new Map implementation:
in part because it seemed unneccessary, but in part also because I feel it would be safer to not expose such operations.

[manovotn]

@Sanne how often does Arc initialize the lazy map we've had before?
It should only happen if the interceptor accesses its bindings - is there some interceptor forcibly doing that for majority of cases?
I guess I am just trying to get the hang of why the previous lazy init was inefficient.

I understand you want to avoid init even in the case of bindings but I don't like that some methods are left out. Technically, that's against specification.

[Ladicek]

Nit: missing + "'" at the end, here and on 2 more places below.

[Sanne]

👍 thanks!

[Ladicek]

Disallowing calling some methods (and/or with some parameters) on the context data map is an interesting idea, as it protects interceptors from stepping on each others toes too much. Too bad the specification doesn't really say much about how this map should work. What's in this PR seems reasonable to me.

[manovotn]

Disallowing calling some methods (and/or with some parameters) on the context data map is an interesting idea, as it protects interceptors from stepping on each others toes too much. Too bad the specification doesn't really say much about how this map should work. What's in this PR seems reasonable to me.

Note that the specification says:

The InvocationContext instance allows an interceptor method to save information in the Map obtained via the getContextData method. This information can subsequently be retrieved and/or updated by other interceptor methods in the invocation chain, and thus serves as a means to pass contextual data between interceptors.

Hence it actually allows them to step on each others toes to some extent.

[Ladicek]

Right, I mean, sure, the entire purpose of the context data map is for 2 interceptors to be able to exchange information. But they need to be aware of each other, which I assume typically means there's a fixed set of keys the 2 interceptors know and use. But if an interceptor tries to access the whole key set / value collection of the context data map, that is suspicious.

[manovotn]

Yes, but the spec doesn't limit that either.
It it true however that I have only seen values/keySet used in debugging (where it is pretty handy to inspect all of the data inside)

[Ladicek]

I understand and agree that the spec doesn't restrict the usage of the context data map, so there's a good argument to not restrict it on our side either. At the same time, I believe there's also a good argument to be made to restrict it, and the present PR does a decent job in my opinion. Obviously, Hyrum's law dictates that we shouldn't really change it now :-)

[Sanne]

@Sanne how often does Arc initialize the lazy map we've had before? It should only happen if the interceptor accesses its bindings - is there some interceptor forcibly doing that for majority of cases? I guess I am just trying to get the hang of why the previous lazy init was inefficient.

Right, sorry I sent the draft in a rush and was supposed to give some better explanations after finishing tests.

We were running performance tests again, and noticed a recurring pattern in interceptors; take SmallRyeCurrentThreadContextInterceptor for example - which is applied extensively by default in all reactive applications;

The first lines of code start with:

    @AroundInvoke
    public Object manageCurrentContext(InvocationContext ctx) throws Exception {
        CurrentThreadContext config = null;
        Object binding = ctx.getContextData().get("io.quarkus.arc.interceptorBindings");

First thing it does is to try reading from the getContextData, which makes the fact that the Map for it is being "lazily allocated" today a bit pointless as the initialization is triggered for each read operation as well; not to mention that looking up the interceptor bindings is also very likely. So technically the use of the LazyValue just adds processing overhead and an additional object (the LazyValue instance) over a plain Map, when there's high likelyhood of such read operations to be triggered.

The purpose of this PR is to keep us from initializing the Map if all what's needed is read operations.

Also: since the pattern I see in SmallRyeCurrentThreadContextInterceptor seems rather reasonable, I expect similar things to be performed by many other interceptors. I think it's plausible to assume that many such interceptors will mainly require read operations - which we can optimise for.

Also: this optimisation proposal doesn't have performance drawbacks for the write cases either - it just replaces the LazyValue holder with the custom Map - and the custom Map implementation I'm suggesting here has the same memory impact as the LazyValue approach.
It actually might improve on the current state even when write operations are triggered: the map is smaller.

So I believe it provides a win-win situation, and optimised for the more realistic scenario of getContextData being actually triggered in a non-trivial application.

I understand you want to avoid init even in the case of bindings but I don't like that some methods are left out. Technically, that's against specification.

Right - we can certainly implement the additional methods as well, but while looking into it I had a bad feeling about allowing that - for the same points @Ladicek made.

Please think about it and let me know the vertict - I can certainly implement the missing capabilities so to only focus on the performance optimisation at this stage, but I'm inclined to think this approach is safer.

I understand the purpose is for interceptors to "share" some state but I hope - for sake of our sanity - that even among spec writers there's an implied assumption that only one interceptor is the "owner" of a particular key, so while allowing others to read it, it shouldn't be allowed for a different one to write / replace / delete on a key space they don't own.
N.B. this patch still allows 3rd party interceptors to mess with each other's state - so we're not even breaking this assumption in case others have a different opinion about design: we're only hardening access to our own platform's interceptorBindings.

[Ladicek]

I think most interceptors actually don't use the context data map at all, so the lazy allocation isn't as inefficient as you think. Unfortunately the "current" interceptor binding annotations are not exposed in a better way -- but in the CDI community, we're talking about exposing the interceptor binding annotations directly from the InvocationContext, so that should help in the future. In the meantime, treating that one key in a special way makes a lot of sense to me.

[manovotn]

Thanks for more context Sanne, few more notes:

We were running performance tests again, and noticed a recurring pattern in interceptors; take SmallRyeCurrentThreadContextInterceptor for example - which is applied extensively by default in all reactive applications;

Where does this usage come from? I mean, which project uses the interceptor in question? grep-ing in Quarkus gave me no result and I don't see this in mutiny either yet from your description it sounds like this interceptor is a primary method through which we implement CP in reactive scenarios?

Please think about it and let me know the vertict - I can certainly implement the missing capabilities so to only focus on the performance optimisation at this stage, but I'm inclined to think this approach is safer.

I won't hard press you for that; looks like I am the only one thinking they should be there :)
What we should add though is a javadoc to the map impl class explaining why we do what we do.

[manovotn]

I think most interceptors actually don't use the context data map at all, so the lazy allocation isn't as inefficient as you think.

True, most user-defined interceptors won't care much, this is more used by other frameworks that need to introspect values in their respective bindings. For instance JTA (Narayana) was using this particular feature (if memory serves) which basically means any operations with transactions will trigger it as well.

Unfortunately the "current" interceptor binding annotations are not exposed in a better way -- but in the CDI community, we're talking about exposing the interceptor binding annotations directly from the InvocationContext, so that should help in the future.
In the meantime, treating that one key in a special way makes a lot of sense to me.

+1, I agree it makes sense to special-case this scenario.

[Sanne]

Thanks for all suggestions :) I've pushed a revised version - same basic principles but:

• documented the rationale
• implemented the missing methods (althouh not allowing to mutate the interceptor bindings specifically)

[Sanne]

Thanks for more context Sanne, few more notes:

We were running performance tests again, and noticed a recurring pattern in interceptors; take SmallRyeCurrentThreadContextInterceptor for example - which is applied extensively by default in all reactive applications;

Where does this usage come from? I mean, which project uses the interceptor in question? grep-ing in Quarkus gave me no result and I don't see this in mutiny either yet from your description it sounds like this interceptor is a primary method through which we implement CP in reactive scenarios?

Right, this interceptor from CP is pulled in and activated by default by many Quarkus extensions by default. It's very likely that any application that has any reactive dependencies will have this interceptor on many endpoints, even when users didn't intentionally opt-in or are aware of this. So this is having wide impact.

[franz1981]

LGTM!

[franz1981]

This can be lazily allocated (and reused) as well, given that users cannot modify it?

[Sanne]

yes it could - but I don't think it's worth it: hopefully nobody is using entrySet heavily on this map.

[manovotn]

Looks good

[Ladicek]

Technically if we really want the context data map to be a full map, then entrySet(), keySet() and values() should be write-through views on the original map :-) Also clear() could possibly just set the delegate to null, but I realize I'm just nit-picking here. Ignoring that, it looks fine.

[Sanne]

Technically if we really want the context data map to be a full map, then entrySet(), keySet() and values() should be write-through views on the original map :-)

oh that would certainly be simpler. But are you saying that the interceptors entry doesn't need to be listed? And what about the size() and isEmpty() methods then? I don't think we can ignore the "virtual" entry from the interceptors, but if we do these should probably be consistent.

Also clear() could possibly just set the delegate to null, but I realize I'm just nit-picking here. Ignoring that, it looks fine.

I could do that but it depends on the previous doubt - I think how we treat the "interceptors entry" needs to be consistent.

[Sanne]

ah sorry you were referring to fact one can modify the keyset and see changes reflected in the Map. Right I ignored that.

[mkouba]

I like the idea in general but speaking of optimizations and SmallRyeCurrentThreadContextInterceptor it might be more effective to provide a quarkus-specific version of this interceptor that would simply use io.quarkus.arc.ArcInvocationContext#getInterceptorBindings() instead.

[mkouba]

Is there a reason to use so many raw types in this class? I get a lot of useless warnings in my IDE ;-)