Reduce initial's Arc ManagedContext activation cost #34310

franz1981 · 2023-06-26T11:07:51Z

This is a companion of #34132

On main:

ManagedContext's initial activation requires to retrieve the request context state value (ie RequestContextState instance freshly created) from the Vertx local context which is already known upfront ( 'cause is the first time we set it, so we know already): this pr is saving this lookup
CurrentVertxRequest's already provide a batch set method (thanks @stuartwdouglas for it!!!) we can use to avoid proxies to perform twice a delegate lookup (which involve io/quarkus/arc/impl/RequestContext.getIfActive to happen twice)

franz1981 · 2023-06-26T12:25:13Z

Waiting #34132 to be merged (if merged) before moving with this

.../runtime/src/main/java/io/quarkus/resteasy/reactive/common/runtime/ArcThreadSetupAction.java

independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java

franz1981 · 2023-06-28T09:17:35Z

@mkouba still waiting #34310 to be merged, but we can do the opposite, when everyone is happy with this; I'll rebase the other one, in case

gsmet · 2023-06-28T09:35:29Z

Marking as draft for now as this is still being discussed and CI is overloaded.

mkouba · 2023-06-28T09:41:19Z

independent-projects/arc/runtime/src/main/java/io/quarkus/arc/ManagedContext.java

     */
-    default void activate() {


For the record, this change is binary incompatible. However, the ManagedContext interface is not intended to be implemented outside the ArC core. Therefore, there's a low risk of breaking existing code. If we run into a problem though we should consider adding synthetic bridge methods via https://github.com/dmlloyd/bridger.

franz1981 · 2023-07-03T06:43:56Z

@mkouba ManagedExecutorTest TCK failures are from the latest commits which includes your changes?

mkouba · 2023-07-03T07:10:13Z

@mkouba ManagedExecutorTest TCK failures are from the latest commits which includes your changes?

Well, it's the assert currentContext.get() == null that triggers an assertion error:

Caused by: java.lang.AssertionError
	at io.quarkus.arc.impl.RequestContext.activate(RequestContext.java:138)
	at io.quarkus.arc.runtime.context.ArcContextProvider$ContextSnapshot.begin(ArcContextProvider.java:145)
	at io.smallrye.context.impl.SlowActiveContextState.<init>(SlowActiveContextState.java:29)
	at io.smallrye.context.impl.SlowCapturedContextState.begin(SlowCapturedContextState.java:34)
	at io.smallrye.context.impl.SlowCapturedContextState.begin(SlowCapturedContextState.java:13)
	at io.smallrye.context.impl.wrappers.SlowContextualSupplier.get(SlowContextualSupplier.java:20)
	at io.smallrye.mutiny.operators.uni.builders.UniCreateFromItemSupplier.subscribe(UniCreateFromItemSupplier.java:28)

It seems that the captured ContextState is not valid anymore but the current context is set. I have no idea if this is legal from MP CP POV. But it probably is because the code in io.quarkus.arc.runtime.context.ArcContextProvider.ContextSnapshot.begin() simply tests the validity of the context and passes null if it's not, thus effectively overrides the current context.

@manovotn @FroMage WDYT?

franz1981 · 2023-07-03T07:10:49Z

@mkouba I've run locally the tests and I can confirm it indeed

<<< END Method.completedFutureDependentStagesRunWithContext FAILED
java.util.concurrent.ExecutionException: java.lang.AssertionError
        at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
        at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
        at io.smallrye.context.CompletableFutureWrapper.get(CompletableFutureWrapper.java:159)
        at org.eclipse.microprofile.context.tck.ManagedExecutorTest.completedFutureDependentStagesRunWithContext(ManagedExecutorTest.java:266)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor$1.invoke(QuarkusProtocol.java:87)
        at org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter.execute(LocalTestExecuter.java:57)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
        at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
        at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
        at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
        at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
        at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
        at io.quarkus.arquillian.QuarkusProtocol$QuarkusMethodExecutor.invoke(QuarkusProtocol.java:67)
        at org.jboss.arquillian.container.test.impl.execution.RemoteTestExecuter.execute(RemoteTestExecuter.java:103)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
        at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
        at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
        at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
        at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
        at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
        at org.jboss.arquillian.container.test.impl.execution.ClientTestExecuter.execute(ClientTestExecuter.java:52)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
        at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
        at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
        at org.jboss.arquillian.container.test.impl.client.ContainerEventController.createContext(ContainerEventController.java:128)
        at org.jboss.arquillian.container.test.impl.client.ContainerEventController.createTestContext(ContainerEventController.java:118)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
        at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
        at org.jboss.arquillian.test.impl.TestContextHandler.createTestContext(TestContextHandler.java:116)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
        at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
        at org.jboss.arquillian.test.impl.TestContextHandler.createClassContext(TestContextHandler.java:83)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
        at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
        at org.jboss.arquillian.test.impl.TestContextHandler.createSuiteContext(TestContextHandler.java:69)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
        at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
        at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
        at org.jboss.arquillian.test.impl.EventTestRunnerAdaptor.test(EventTestRunnerAdaptor.java:139)
        at org.jboss.arquillian.testng.Arquillian.run(Arquillian.java:138)
        at org.testng.internal.MethodInvocationHelper.invokeHookable(MethodInvocationHelper.java:253)
        at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:594)
        at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:173)
        at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
        at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:824)
        at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:146)
        at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
        at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
        at org.testng.TestRunner.privateRun(TestRunner.java:794)
        at org.testng.TestRunner.run(TestRunner.java:596)
        at org.testng.SuiteRunner.runTest(SuiteRunner.java:377)
        at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:371)
        at org.testng.SuiteRunner.privateRun(SuiteRunner.java:332)
        at org.testng.SuiteRunner.run(SuiteRunner.java:276)
        at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
        at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
        at org.testng.TestNG.runSuitesSequentially(TestNG.java:1212)
        at org.testng.TestNG.runSuitesLocally(TestNG.java:1134)
        at org.testng.TestNG.runSuites(TestNG.java:1063)
        at org.testng.TestNG.run(TestNG.java:1031)
        at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:155)
        at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeSingleClass(TestNGDirectoryTestSuite.java:102)
        at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:91)
        at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:137)
        at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
        at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
        at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
        at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: java.lang.AssertionError
        at io.quarkus.arc.impl.RequestContext.activate(RequestContext.java:132)
        at io.quarkus.arc.ManagedContext.activate(ManagedContext.java:17)
        at io.quarkus.arc.runtime.context.ArcContextProvider$ClearContextSnapshot.begin(ArcContextProvider.java:81)
        at io.smallrye.context.impl.SlowActiveContextState.<init>(SlowActiveContextState.java:29)
        at io.smallrye.context.impl.SlowCapturedContextState.begin(SlowCapturedContextState.java:34)
        at io.smallrye.context.impl.SlowCapturedContextState.begin(SlowCapturedContextState.java:13)
        at io.smallrye.context.impl.wrappers.SlowContextualBiFunction.apply(SlowContextualBiFunction.java:20)
        at java.base/java.util.concurrent.CompletableFuture.biApply(CompletableFuture.java:1311)
        at java.base/java.util.concurrent.CompletableFuture$BiApply.tryFire(CompletableFuture.java:1280)
        at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510)
        at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2179)
        at org.eclipse.microprofile.context.tck.ManagedExecutorTest.completedFutureDependentStagesRunWithContext(ManagedExecutorTest.java:264)
        ... 81 more

FroMage · 2023-07-04T14:36:40Z

I really don't know how this class works, it's mostly @manovotn who understands it :)

franz1981 · 2023-07-04T15:42:46Z

@mkouba if we don't need to keep the assertion I can remove it, in case, and instead, open an issue where we need to understand better the life-cycle of it

mkouba · 2023-07-07T08:09:34Z

@mkouba if we don't need to keep the assertion I can remove it, in case, and instead, open an issue where we need to understand better the life-cycle of it

Yep, that sounds reasonable. Let's comment out that assertion for now and file a new tracking issue.

franz1981 · 2023-07-07T10:05:39Z

@mkouba assertion removed!
wdyt, are we ready to go?

franz1981 · 2023-07-11T11:47:09Z

@mkouba assertion removed, do I need to squash the changes?

mkouba · 2023-07-12T10:03:13Z

@mkouba assertion removed, do I need to squash the changes?

Yes please ;-)

franz1981 · 2023-07-12T14:06:07Z

@mkouba squashing done! (Sorry I've lost you as co-author :/ )

mkouba · 2023-07-13T08:16:31Z

@mkouba squashing done! (Sorry I've lost you as co-author :/ )

No problem. I think that the only thing you need to do is to add something like Co-authored-by: NAME <[email protected]> in the comment.

geoand · 2023-07-13T08:17:29Z

I think that the only thing you need to do is to add something like Co-authored-by: NAME [email protected] in the comment.

That is correct - it's simply a matter of editing the commit message and adding this

Co-authored-by: [email protected]

franz1981 · 2023-07-14T09:58:11Z

@mkouba @geoand done!

quarkus-bot · 2023-07-14T16:54:48Z

Failing Jobs - Building `ccabce0`

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
⌛	JVM Tests - JDK 17	`Build`	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 19

- follow-up to quarkusio#34310

- follow-up to quarkusio#34310 (cherry picked from commit 335651b)

quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/rest labels Jun 26, 2023

franz1981 commented Jun 26, 2023

View reviewed changes

.../runtime/src/main/java/io/quarkus/resteasy/reactive/common/runtime/ArcThreadSetupAction.java Show resolved Hide resolved

franz1981 changed the title ~~Reduce initial's Arc ManagedContext initial activation cost~~ Reduce initial's Arc ManagedContext activation cost Jun 26, 2023

franz1981 force-pushed the ctx branch from e17bedb to 257ea68 Compare June 27, 2023 08:36

franz1981 commented Jun 27, 2023

View reviewed changes

independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java Outdated Show resolved Hide resolved

franz1981 commented Jun 28, 2023

View reviewed changes

independent-projects/arc/runtime/src/main/java/io/quarkus/arc/impl/RequestContext.java Outdated Show resolved Hide resolved

franz1981 marked this pull request as ready for review June 28, 2023 09:16

gsmet marked this pull request as draft June 28, 2023 09:35

mkouba reviewed Jun 28, 2023

View reviewed changes

franz1981 force-pushed the ctx branch 2 times, most recently from a1a1744 to 965f906 Compare June 30, 2023 13:55

geoand requested review from Ladicek and mkouba June 30, 2023 13:57

geoand marked this pull request as ready for review June 30, 2023 13:59

mkouba approved these changes Jun 30, 2023

View reviewed changes

Ladicek approved these changes Jun 30, 2023

View reviewed changes

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 30, 2023

This comment has been minimized.

Sign in to view

franz1981 force-pushed the ctx branch from 965f906 to 65931d5 Compare July 4, 2023 09:33

franz1981 force-pushed the ctx branch from 7233bac to 6de045c Compare July 7, 2023 10:05

franz1981 mentioned this pull request Jul 7, 2023

ManagedExecutorTest TCK override current context: is ti valid? #34598

Closed

This comment has been minimized.

Sign in to view

franz1981 force-pushed the ctx branch from 6de045c to d825a59 Compare July 12, 2023 14:02

quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/mongodb area/platform Issues related to definition and interaction with Quarkus Platform area/testing labels Jul 12, 2023

franz1981 force-pushed the ctx branch from d825a59 to 91aff3f Compare July 12, 2023 14:05

Save ContextState lookup on ManagerContext's initial activation

ccabce0

Co-authored-by: [email protected]

franz1981 force-pushed the ctx branch from 91aff3f to ccabce0 Compare July 14, 2023 09:53

geoand merged commit 0b0eff7 into quarkusio:main Jul 19, 2023

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 19, 2023

quarkus-bot bot added this to the 3.3 - main milestone Jul 19, 2023

mkouba added a commit to mkouba/quarkus that referenced this pull request Aug 29, 2023

ArC: fix binary compatibility problem with ManagedContext

7070a02

- follow-up to quarkusio#34310

mkouba mentioned this pull request Aug 29, 2023

ArC: fix binary compatibility problem with ManagedContext #35615

Merged

mkouba added a commit to mkouba/quarkus that referenced this pull request Aug 29, 2023

ArC: fix binary compatibility problem with ManagedContext

335651b

- follow-up to quarkusio#34310

gsmet pushed a commit to gsmet/quarkus that referenced this pull request Sep 4, 2023

ArC: fix binary compatibility problem with ManagedContext

9c75237

- follow-up to quarkusio#34310 (cherry picked from commit 335651b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce initial's Arc ManagedContext activation cost #34310

Reduce initial's Arc ManagedContext activation cost #34310

franz1981 commented Jun 26, 2023 •

edited

Loading

franz1981 commented Jun 26, 2023

franz1981 commented Jun 28, 2023

gsmet commented Jun 28, 2023

mkouba Jun 28, 2023

This comment has been minimized.

franz1981 commented Jul 3, 2023

mkouba commented Jul 3, 2023

franz1981 commented Jul 3, 2023

FroMage commented Jul 4, 2023

franz1981 commented Jul 4, 2023

mkouba commented Jul 7, 2023

franz1981 commented Jul 7, 2023

franz1981 commented Jul 11, 2023

This comment has been minimized.

mkouba commented Jul 12, 2023

franz1981 commented Jul 12, 2023

mkouba commented Jul 13, 2023

geoand commented Jul 13, 2023

franz1981 commented Jul 14, 2023

quarkus-bot bot commented Jul 14, 2023

Reduce initial's Arc ManagedContext activation cost #34310

Reduce initial's Arc ManagedContext activation cost #34310

Conversation

franz1981 commented Jun 26, 2023 • edited Loading

franz1981 commented Jun 26, 2023

franz1981 commented Jun 28, 2023

gsmet commented Jun 28, 2023

mkouba Jun 28, 2023

Choose a reason for hiding this comment

This comment has been minimized.

franz1981 commented Jul 3, 2023

mkouba commented Jul 3, 2023

franz1981 commented Jul 3, 2023

FroMage commented Jul 4, 2023

franz1981 commented Jul 4, 2023

mkouba commented Jul 7, 2023

franz1981 commented Jul 7, 2023

franz1981 commented Jul 11, 2023

This comment has been minimized.

mkouba commented Jul 12, 2023

franz1981 commented Jul 12, 2023

mkouba commented Jul 13, 2023

geoand commented Jul 13, 2023

franz1981 commented Jul 14, 2023

quarkus-bot bot commented Jul 14, 2023

Failing Jobs - Building ccabce0

franz1981 commented Jun 26, 2023 •

edited

Loading

Failing Jobs - Building `ccabce0`