Lazily initialize TransportActionStats in RequestHandlerRegistry #117435
Conversation
Same as #114107 but for the transport side. We consuming more than 0.5M for these stat instances and a large fraction if not the majority will go unused in many use-cases, given how expensive stats are in terms of volatile operations the overhead of an added `getAquire` is trivial in the absence of writes.
original-brownbear
added
>non-issue
:Data Management/Stats
elasticsearchmachine
added
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Pinging @elastic/es-data-management (Team:Data Management) |
| 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; |
There was a problem hiding this comment.
If it's volatile we don't need to use STATS_TRACKER_HANDLE.getAcquire(this) to read it do we?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure thing, dded this one as well as MethodHandlers where I did the same thing now.
| } | ||
|
|
||
| private TransportActionStatsTracker statsTracker() { | ||
| var tracker = existingStatsTracker(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
original-brownbear
added
auto-merge-without-approval
💚 Backport successful
|
…7435) (#118381) Same as #114107 but for the transport side. We are consuming more than 0.5M for these stat instances and a large fraction if not the majority will go unused in many use-cases, given how expensive stats are in terms of volatile operations the overhead of an added `getAcquire` is trivial in the absence of writes. Outside of reducing the prod footprint this is also kind of nice in x-pack internal cluster tests in particular where it saves quite a bit of heap. Co-authored-by: Dimitris Rempapis <[email protected]>
3 participants
Same as #114107 but for the transport side.
We are consuming more than 0.5M for these stat instances and a large fraction if not the majority will go unused in many use-cases, given how expensive stats are in terms of volatile operations the overhead of an added
getAcquireis trivial in the absence of writes.Outside of reducing the prod footprint this is also kind of nice in x-pack internal cluster tests in particular where it saves quite a bit of heap.