Make some constant SubscribableListener instances cheaper #124452
Conversation
We can just use a real constant for the `null` case, avoiding any non-plain stores in all cases. This should be somewhat helpful for the security interceptors. Also, the spots where we create completed instances can use weaker release-type memory semantics to potentially help the compiler out.
original-brownbear
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| * Create a {@link SubscribableListener} which has already succeeded with the given result. | ||
| */ | ||
| public static <T> SubscribableListener<T> newSucceeded(T result) { | ||
| var res = new SubscribableListener<T>(); |
There was a problem hiding this comment.
This allocates a new Object() for the initial state, can we find a way not to do that?
There was a problem hiding this comment.
Ah 🤦 gotta fix that too, otherwise this is kinda pointless. Sec on it
There was a problem hiding this comment.
Alright, not sure you'll like this one :) But if I get rid of EMPTY this all falls into place just fine doesn't it?
| */ | ||
| public static <T> SubscribableListener<T> newSucceeded(T result) { | ||
| var res = new SubscribableListener<T>(); | ||
| VH_STATE_FIELD.setRelease(res, new SuccessResult<>(result)); |
There was a problem hiding this comment.
Hm so my understanding of setRelease is a little sketchy but AIUI it blocks earlier stores/loads from moving later but does not block later stores/loads from moving earlier. Is that valid here? It seems like if we did something like this ...
l = SubscribableListener.newSucceeded(r1);
// make l visible to another thread
l.onResponse(r2);
... then potentially someone could observe r2 rather than r1?
OTOH if there's something magic about newly-constructed objects that makes this work, does the same reasoning not also work for all volatile writes in a constructor (as long as it doesn't have let this escape I guess)? If so, why is the compiler not already doing this?
Basically this feels risky enough that I'd like to see hard data about the benefits, and a big old comment proving its correctness.
There was a problem hiding this comment.
This will not be a problem because you do a volatile mode read on the state field in onResponse to check if it isn't done already. So that later store never actually happens here.
does the same reasoning not also work for all volatile writes in a constructor
There is no magic for newly constructed objects actually. See https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-volatiles-are-finals :)
Basically this feels risky enough that I'd like to see hard data about the benefits,
That's probably impossible to come by :) It's dependant on target architecture, JVM version and all of that stuff.
For me it's just, the more we use this primitive, the more likely we are to create a spot where it's advantageous to have some operations reordered to before the creation of these already-complete instances. Also as compilers make progress this becomes more likely :)
test/framework/src/main/java/org/elasticsearch/test/TestCluster.java
Outdated
Show resolved
Hide resolved
|
Sure thing, backed everything else out for now :) That's the only change that I can see a measurable improvement on transport threads for :) (from security, could probably take this further and add some more constants in security in a follow-up) |
original-brownbear
added
the
auto-backport
|
Thanks David! |
💔 Backport failed
You can use sqren/backport to manually backport by running |
We can just use a real constant for the
nullcase, avoiding any non-plain stores in all cases. This should be somewhat helpful for the security interceptors.Also, the spots where we create completed instances can use weaker release-type memory semantics to potentially help the compiler out.