Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArC: performance optimizations related to client proxy invocations #36626

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Oct 23, 2023

The first commit introduces the ContextInstances abstraction and generates the implementations for application and request contexts by default. The main goal is to avoid ConcurrentHashMap lookups in the hot path (i.e. for every client proxy invocation).

The generated implementation of an application context for config-quickstart looks like:
https://gist.github.com/mkouba/df6526fe8f3c76d4b773b814e1359555

Our benchmarks show that for application context the throughput has increased significantly: I've observed ~ 75% for 1 bean and ~ 25% for 200 beans. The size of the generated class is ~ 3KB for one bean and ~ 200KB for 200 beans. The numbers are lower for request context because in this case the container has to do much more (e.g. check if the context is active). This optimization can be disabled with quarkus.arc.optimize-contexts=false.

The second commit is similar to how we optimize the client proxy delegate access for the application context. However for the request context, we can only do this optimization if a single request context implementation is registered. I've observed ~ 20% improvement in the RequestScopedProxyInvocationBenchmark.

@mkouba mkouba requested review from Ladicek, manovotn and Sanne and removed request for Ladicek and manovotn October 23, 2023 09:03
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Oct 23, 2023
@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my ordinary nitpicking, nothing unusual.

The 2nd commit I believe we could apply to all normal scopes for which only 1 context is registered. It makes sense to keep ClientProxies.getApplicationScopedDelegate() separate, because we know that the application context is always active (and so the implementation is more performant); for other contexts, where we either don't know at all or we know that the context may be inactive, the getRequestScopedDelegate() implementation would be appropriate.

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to give my feedback mostly over Zulip so just a few bits here.
Overall, I am not a big fan of replacing a nice API such as Map with this generated class, but I understand the motivation and the results seem to support it, so +1.

The 2nd commit I believe we could apply to all normal scopes for which only 1 context is registered. It makes sense to keep ClientProxies.getApplicationScopedDelegate() separate, because we know that the application context is always active (and so the implementation is more performant); for other contexts, where we either don't know at all or we know that the context may be inactive, the getRequestScopedDelegate() implementation would be appropriate.

I also agree with Ladislav in this point - we can apply the same logic to any normal scope and the optimization will be applicable in majority of cases as having multiple scope implementations is rare.

- introduce the ContextInstances abstraction
- if optimization is enabled then generate the ContextInstances
  implementation for application and request context
- for normal scopes for which a single context is registered
- similar to how we optimize the client proxy delegate access for
application context
@mkouba
Copy link
Contributor Author

mkouba commented Oct 24, 2023

The 2nd commit I believe we could apply to all normal scopes for which only 1 context is registered.

I also agree with Ladislav in this point - we can apply the same logic to any normal scope and the optimization will be applicable in majority of cases as having multiple scope implementations is rare.

@Ladicek @manovotn Done.

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 24, 2023
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Oct 26, 2023

@Ladicek @manovotn you all good with Martin's latest changes? If so, we can merge!

@manovotn
Copy link
Contributor

@Ladicek @manovotn you all good with Martin's latest changes? If so, we can merge!

Yea, all good 👍

Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, my only comment is nitpicking :-)

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 27, 2023

Failing Jobs - Building d6c9f73

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 21 Build Failures Logs Raw logs
Native Tests - Windows - RESTEasy Jackson Setup GraalVM ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 21 #

- Failing: integration-tests/virtual-threads/mailer-virtual-threads 

📦 integration-tests/virtual-threads/mailer-virtual-threads

io.quarkus.virtual.mail.RunOnVirtualThreadTest.test line 25 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:278)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:304)

@mkouba mkouba merged commit 8459534 into quarkusio:main Oct 30, 2023
48 of 51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 30, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants