Skip to content

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Dec 31, 2024

When initializing FastThreadLocal eagerly VertxCurrentContextFactory.VertxCurrentContext we were inadvertently forcing the initialization of the logging infrastructure of Netty far too early.

This can be seen in the following flamegraph (in purple on the right):

Screenshot from 2024-12-31 12-10-28

This change shouldn't have much of an impact on its own, but it's a good thing as it moves the initialization of the Netty logging out of the main thread

@franz1981
Copy link
Contributor

franz1981 commented Dec 31, 2024

The supplier get can keep on creating new fast thread locals?

I am a bit surprised we have a non static final fast thread local there tbh, regardless...
Fast thread locals allocations move an atomic int sequence forward, for each created instance, causing a global overall cost to increase, so creation of these should be as limited as possible

eclipse-vertx/vert.x#5078 for more info

@geoand
Copy link
Contributor Author

geoand commented Dec 31, 2024

The supplier get can keep on creating new fast thread locals?

Yes, as I did not want to change the existing behavior.
We probably should change it along the lines you mention, but that is something to be done as a follow up, and something that @mkouba needs to sign off on.

FWIW, under normal circumstances we won't even need the FastThreadLocal (as its name implies)

This comment has been minimized.

This comment has been minimized.

@geoand geoand requested a review from mkouba January 3, 2025 07:22
@mkouba
Copy link
Contributor

mkouba commented Jan 3, 2025

This change shouldn't have much of an impact on its own, but it's a good thing as it moves the initialization of the Netty logging out of the main thread

Well, it can slow down the fallback path a little bit because LazyValue wraps a volatile but it should not really matter because in most cases the fallback/FastThreadLocal is not used at all (i.e. the Vertx duplicated context is available in most cases).

I am a bit surprised we have a non static final fast thread local there tbh, regardless...

It's ok here. There's only one Context object created per scope.

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 3, 2025
Copy link

quarkus-bot bot commented Jan 3, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5e46eb5.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/opentelemetry/deployment

io.quarkus.opentelemetry.deployment.traces.OpenTelemetrySpanSecurityEventsTest.testSecurityEventTypes - History

  • event executor terminated - java.util.concurrent.RejectedExecutionException
java.util.concurrent.RejectedExecutionException: event executor terminated
	at io.netty.util.concurrent.SingleThreadEventExecutor.reject(SingleThreadEventExecutor.java:934)
	at io.netty.util.concurrent.SingleThreadEventExecutor.offerTask(SingleThreadEventExecutor.java:353)
	at io.netty.util.concurrent.SingleThreadEventExecutor.addTask(SingleThreadEventExecutor.java:346)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:836)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute0(SingleThreadEventExecutor.java:827)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:817)
	at io.vertx.core.impl.EventLoopExecutor.execute(EventLoopExecutor.java:35)

@geoand geoand merged commit 4f06e76 into quarkusio:main Jan 3, 2025
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Jan 3, 2025
@geoand geoand deleted the arc-netty branch January 3, 2025 14:28
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants