Skip to content

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Dec 10, 2024
Merged

Lazily initialize TransportActionStats in RequestHandlerRegistry #117435

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d5cfb79
Lazily initialize TransportActionStats in RequestHandlerRegistry
original-brownbear Nov 23, 2024
9366534
Merge branch 'main' into lazy-stats
original-brownbear Nov 26, 2024
a6d0c5d
Merge remote-tracking branch 'elastic/main' into lazy-stats
original-brownbear Dec 10, 2024
7e75864
remove volatile
original-brownbear Dec 10, 2024
e61db28
CR review
original-brownbear Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions server/src/main/java/org/elasticsearch/transport/RequestHandlerRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 volatile TransportActionStatsTracker statsTracker;
Copy link
Contributor

@DaveCTurner DaveCTurner Dec 10, 2024

Choose a reason for hiding this comment

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

If it's volatile we don't need to use STATS_TRACKER_HANDLE.getAcquire(this) to read it do we?

Copy link
Contributor Author

@original-brownbear original-brownbear Dec 10, 2024

Choose a reason for hiding this comment

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

Right but lets drop the volatile then :D I figured I'd be fun to experiment with weaker ordering a little here and there in obvious spots, might be helpful on ARM in some cases where there's only ever a single write to a variable. Who knows might help init performance either now or on some future ARM or similar hardware.
(admittedly there's more interesting spots for that stuff than this ... :))


private static final VarHandle STATS_TRACKER_HANDLE;

static {
Copy link
Contributor

@DaveCTurner DaveCTurner Dec 10, 2024

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#ensureInitialized to ensure it's always initialized eagerly before the security manager comes along and causes problems?

Copy link
Contributor Author

@original-brownbear original-brownbear Dec 10, 2024

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 MethodHandlers where I did the same thing now.

try {
STATS_TRACKER_HANDLE = MethodHandles.lookup()
.findVarHandle(RequestHandlerRegistry.class, "statsTracker", TransportActionStatsTracker.class);
} catch (Exception e) {
throw new ExceptionInInitializerError(e);
}
}

public RequestHandlerRegistry(
String action,
Expand Down Expand Up @@ -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();
Copy link
Contributor

@DaveCTurner DaveCTurner Dec 10, 2024

Choose a reason for hiding this comment

The 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 compareAndExchange will establish the right guarantees, and if it's not null then it has to be the right value.

Risks perhaps creating a few more unused TransportActionStatsTracker instances tho I guess.

Copy link
Contributor Author

@original-brownbear original-brownbear Dec 10, 2024

Choose a reason for hiding this comment

The 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);
}
}
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/transport/TransportActionStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ public record TransportActionStats(
long[] responseSizeHistogram
) implements Writeable, ToXContentObject {

public static final TransportActionStats EMPTY = new TransportActionStats(0, 0, new long[0], 0, 0, new long[0]);

public TransportActionStats(StreamInput in) throws IOException {
this(in.readVLong(), in.readVLong(), in.readVLongArray(), in.readVLong(), in.readVLong(), in.readVLongArray());
}
Expand Down