Request scope performance improvements

[stuartwdouglas]

Previously this required 3 ThreadLocal operations:

• isActive()
• create(Contextual)
• create(Contextual, CreationalContext)

This PR provides a single operation so that these can
be accomplished with only a single ThreadLocal op

[mkouba]

I really like the optimization of ArcContainerImpl#getActiveContext() - this will definitely save one ArrayList allocation in 90% scenarios. So +100!

I'm not so sure about the other changes.

Previously we did:

1. Find the context (one context.isActive() volatilethread-local read)
2. Try RequestContext.get(Contextual<T>) first (one currentContext.get() volatilethread-local read) -> final operation once the bean was already created
3. Call RequestContext.get(Contextual<T>,CreationalContext) (one currentContext.get() volatilethread-local read) -> only used the first time the bean was accessed

In this PR we do:

1. Call ArcContainerImpl.normalScopedInstance() (one context.isActive() volatilethread-local read)
2. Call RequestContext.getOrCreate(Contextual<T>) (one currentContext.get() volatilethread-local read)

So with this new approach you will only save one volatilethread-local read plus few method invocations during the first invocation upon a @RequestScoped bean and the changes include a lot of duplicit code which is harder to maintain and understand.

Also is there a perf bottleneck use case we're trying to solve? Or is it purely a hunt for better results in a perf benchmark?

[mkouba]

CC @manovotn

[stuartwdouglas]

They are ThreadLocal reads that are the main issue rather than volatile ones.

There is no isActive in ArcContainerImpl.normalScopedInstance() call in normal circumstances, this will only happen if you have two contexts backing the same scope. Basically once the bean has been created, if there is another scope it could come from this is checked if active.

I am mostly looking at the setup of CurrentVertxRequest, as something that happens every request and is not always used, at the moment this is 3 thread local ops, this changes it to one.

[mkouba]

They are ThreadLocal reads that are the main issue rather than volatile ones.

Oh, sorry... yes I was talking about thread local reads of course.

There is no isActive in ArcContainerImpl.normalScopedInstance()

I see.

I am mostly looking at the setup of CurrentVertxRequest

Hm, the CurrentVertxRequest does not look like a well designed construct. First of all, the CurrentVertxRequest.current must not be public - field access on a client proxy will always return null. Furthermore, for the price of two @RequestScoped beans we hide the fact that a thread local is used under the hood anyway.

I'd recommend to change the CurrentVertxRequest.current to a thread local and remove the @RequestScoped from the CurrentVertxRequest class and make the producer static. This way each CurrentVertxRequest.setCurrent(RoutingContext) will be a thread local write and no bean is created unless the RoutingContext is injected and used:

@Dependent // just to make sure the static producer is discovered
class CurrentVertxRequest {

    private static final ThreadLocal<RoutingContext> current;

    @Produces
    @RequestScoped
    public static RoutingContext getCurrent() {
        return current.get();
    }

    public void setCurrent(RoutingContext current) {
       this.current.set(current);
    }
}

@stuartwdouglas WDYT?

[stuartwdouglas]

They are ThreadLocal reads that are the main issue rather than volatile ones.

Oh, sorry... yes I was talking about thread local reads of course.

There is no isActive in ArcContainerImpl.normalScopedInstance()

I see.

I am mostly looking at the setup of CurrentVertxRequest

Hm, the CurrentVertxRequest does not look like a well designed construct. First of all, the CurrentVertxRequest.current must not be public - field access on a client proxy will always return null. Furthermore, for the price of two @RequestScoped beans we hide the fact that a thread local is used under the hood anyway.

That was an oversight, the public field is not actually used anywhere.

I'd recommend to change the CurrentVertxRequest.current to a thread local and remove the @RequestScoped from the CurrentVertxRequest class and make the producer static. This way each CurrentVertxRequest.setCurrent(RoutingContext) will be a thread local write and no bean is created unless the RoutingContext is injected and used:

Then you need to implement MP context propagation for our new thread local. If you start doing context propagation you are now propagating two contexts instead of one. You also then need to manage the lifecycle of your new ThreadLocal so that everywhere that would start or stop the request context now also needs to deal with our custom ThreadLocal instead.

This is request scoped data, it belongs in the request scope, we should not be implementing our own custom ThreadLocal versions of this.

@Dependent // just to make sure the static producer is discovered
class CurrentVertxRequest {

    private static final ThreadLocal<RoutingContext> current;

    @Produces
    @RequestScoped
    public static RoutingContext getCurrent() {
        return current.get();
    }

    public void setCurrent(RoutingContext current) {
       this.current.set(current);
    }
}

@stuartwdouglas WDYT?

[mkouba]

I agree that the RoutingContext bean should live in the request context. What I'm disputing is the existence of the CurrentVertxRequest which is more or less a useless RoutingContext holder for which we generate a bean metadata class, client proxy, and then call the proxy to store the value etc.

Then you need to implement MP context propagation for our new thread local.

That's a good point. MP context propagation uff... in order to make it work we must set the current RoutingContext eagerly (which is quite stupid).

In that case, I don't have any other arguments, except that I find the changes not worth the "tangle" (from maintainability POV).

Do you have some perf numbers?

[stuartwdouglas]

It made a measurable difference to perf.

IMHO this makes things simpler rather than more complex as it removes the logic from the generated bytecode in the client proxies, and into a normal method. This also makes it much easier to step into a debug client proxy calls.

[gsmet]

So what do we do for this one? :)

[mkouba]

I need to go through the code again and try to find some compromise. I'm -1 for merging as it is though. "measurable difference" is a good argument (although quite vague) but I'd rather avoid changes like this just because we want to improve some benchmark a little bit...

[mkouba]

I've tried to keep the original idea but polish the API a little bit (e.g. rename the InjectableContext#getOrCreate() because the Context.get(Contextual<T>, CreationalContext<T>) method has a very similar contract, remove the usage of the CreationalContextImpl from the API, moved the logic from ArcContainer.normalScopedInstance() to a util class, etc.).

I think it's a good compromise. It would be great if @stuartwdouglas can validate that this modified version still makes a measurable difference. @manovotn pls verify the API and correctness ;-).

[mkouba]

Ah, there is a formatting problem again...

[mkouba]

The test failure is related...

[mkouba]

Ok, the PR is now ready for final review. @manovotn @stuartwdouglas

[manovotn]

Looks good. There should still be the perf improvement since you are leveraging getIfActive but I am +1 to have Stuart glance at this as well.