-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Lazily initialize TransportActionStats in RequestHandlerRegistry #117435
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
Changes from all commits
d5cfb79
9366534
a6d0c5d
7e75864
e61db28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ | |
| import org.elasticsearch.telemetry.tracing.Tracer; | ||
|
|
||
| import java.io.IOException; | ||
| import java.lang.invoke.MethodHandles; | ||
| import java.lang.invoke.VarHandle; | ||
| import java.util.concurrent.Executor; | ||
|
|
||
| import static org.elasticsearch.core.Releasables.assertOnce; | ||
|
|
@@ -33,7 +35,19 @@ public class RequestHandlerRegistry<Request extends TransportRequest> implements | |
| private final TaskManager taskManager; | ||
| private final Tracer tracer; | ||
| private final Writeable.Reader<Request> requestReader; | ||
| private final TransportActionStatsTracker statsTracker = new TransportActionStatsTracker(); | ||
| @SuppressWarnings("unused") // only accessed via #STATS_TRACKER_HANDLE, lazy initialized because instances consume non-trivial heap | ||
| private TransportActionStatsTracker statsTracker; | ||
|
|
||
| private static final VarHandle STATS_TRACKER_HANDLE; | ||
|
|
||
| static { | ||
| try { | ||
| STATS_TRACKER_HANDLE = MethodHandles.lookup() | ||
| .findVarHandle(RequestHandlerRegistry.class, "statsTracker", TransportActionStatsTracker.class); | ||
| } catch (Exception e) { | ||
| throw new ExceptionInInitializerError(e); | ||
| } | ||
| } | ||
|
|
||
| public RequestHandlerRegistry( | ||
| String action, | ||
|
|
@@ -118,15 +132,34 @@ public static <R extends TransportRequest> RequestHandlerRegistry<R> replaceHand | |
| } | ||
|
|
||
| public void addRequestStats(int messageSize) { | ||
| statsTracker.addRequestStats(messageSize); | ||
| statsTracker().addRequestStats(messageSize); | ||
| } | ||
|
|
||
| @Override | ||
| public void addResponseStats(int messageSize) { | ||
| statsTracker.addResponseStats(messageSize); | ||
| statsTracker().addResponseStats(messageSize); | ||
| } | ||
|
|
||
| public TransportActionStats getStats() { | ||
| var statsTracker = existingStatsTracker(); | ||
| if (statsTracker == null) { | ||
| return TransportActionStats.EMPTY; | ||
| } | ||
| return statsTracker.getStats(); | ||
| } | ||
|
|
||
| private TransportActionStatsTracker statsTracker() { | ||
| var tracker = existingStatsTracker(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're looking for yet-weaker memory semantics, I think this one can be a regular (nonvolatile) read. If it's null then the Risks perhaps creating a few more unused
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea somehow a plain read in this kind of spot weirds me out a little :D but you are right, we could relax these spots further technically I guess I just don't feel adventurous enough for that this evening (I do realise though that we actually do this kind of thing ... sync write -> plain read ... in a couple spots like the CS + routing table computation already). |
||
| if (tracker == null) { | ||
| var newTracker = new TransportActionStatsTracker(); | ||
| if ((tracker = (TransportActionStatsTracker) STATS_TRACKER_HANDLE.compareAndExchange(this, null, newTracker)) == null) { | ||
| tracker = newTracker; | ||
| } | ||
| } | ||
| return tracker; | ||
| } | ||
|
|
||
| private TransportActionStatsTracker existingStatsTracker() { | ||
| return (TransportActionStatsTracker) STATS_TRACKER_HANDLE.getAcquire(this); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this to the classes passed to
org.elasticsearch.bootstrap.Elasticsearch#ensureInitializedto ensure it's always initialized eagerly before the security manager comes along and causes problems?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, dded this one as well as
MethodHandlerswhere I did the same thing now.