-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Optimize ImmutableOpenMap.Builder #85184
Optimize ImmutableOpenMap.Builder #85184
Conversation
Optimizes the case of a noop builder -- builder(someMap).build() becomes essentially free. The real win here is when the constructing of the builder and the final invocation of build are separated by lots of conditionals and the runtime behavior happens to be that the builder is a noop.
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Hi @joegallo, I've created a changelog YAML for you. |
5f51021
to
d900342
Compare
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.
LGTM, benchmarked this and it offers some unexpected speedups indeed by avoiding copying the large indices
map in the metadata builder whenever we're updating a separate part of the metadata (I think that's where most of the speedups are from). Thanks Joe!
server/src/main/java/org/elasticsearch/common/collect/ImmutableOpenMap.java
Outdated
Show resolved
Hide resolved
"He must have died while carving it."
to make it clearer that this is *not* the same as the other 'map'
Related to #77466 (also kindof related to #82708, in that that's what got me thinking about this). Also shoutout to @jakelandis and also @original-brownbear for daydreaming about this with me earlier today.
Consider
Metadata.Builder
, it builds fourImmutableOpenMap.Builder
s from fourImmutableOpenMap
s:elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/Metadata.java
Lines 1186 to 1189 in 21a5a55
In the current code (before this branch), each of those
builder(someMap)
invocations results in anObjectObjectHashMap#clone()
behind the scenes. Then, later, thoseObjectObjectHashMap
clones are used to construct newImmutableOpenMap
s inMetadata.Builder#build()
. None of that is predicated on actually touching those maps, soMetadata.builder(metadata).version(2).build()
will clone and copy four maps.This PR changes that in two ways. First, it makes a no-op trip of some map through an
ImmutableOpenMap.Builder
return the exact same reference, somyMap == ImmutableOpenMap.builder(myMap).build()
. Second, it defersclone
ing of a passed-in map until the point where a mutable map reference is needed for either a read or a write, so the runtime cost in memory and cpu of a no-op trip through a builder is essentially zero.Given the way that we frequently use
ImmutableOpenMap.Builder
s in otherBuilder
s to refer to maps that often do not change (seegrep
output that follows), this should make many cluster state updates quite a bit cheaper.For example, in a very ordinary ILM cluster state update, we change the
customMetadata
on anIndexMetadata
but not thealiases
or therolloverInfos
, and then we put that into a newMetadata
so we'd be changing theindices
but not thetemplates
orcustoms
. Currently, cluster state updates that touch that kind of object tree are paying a price for these things whether they change anything in them or not. With this PR, all those things we don't touch now cost nothing.