MasterService#patchVersions is rather inefficient #77888
Labels
>bug
:Distributed Coordination/Cluster Coordination
Cluster formation and cluster state publication, including cluster membership and fault detection.
Team:Distributed (Obsolete)
Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination.
Comments
DaveCTurner
added
>bug
:Distributed Coordination/Cluster Coordination
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
Link #77466 This one is still very interesting to the many shards effort. I just looked at a recent profile and as we make improvements to various CS update steps this is actually starting to make up a larger and larger portion of what the master update thread spends its time on. |
This was referenced Oct 11, 2021
|
Looks like #79004 is going to mostly address this now from the looks of it (I just benchmarked it and that PR gets us a large chunk of the obvious improvements that are possible for this method). Please hold off on working on this for now. |
martijnvg
added a commit
that referenced
this issue
Oct 26, 2021
In cases when indices, aliases and data streams aren't modified then the indices lookup can be reused. For example in: * The IndexMetadataUpdater#applyChanges(...) method builds a new metadata instance, but only primary term or insync allocations may be updated. No new indices, aliases or data streams are added, so re-building indices lookup is not necessary. * MasterService#patchVersions Additionally the logic that checks when indices lookup can be reused, this logic also checks the hidden and system flags of indices/datastreams. In clusters with many indices the cost of building indices lookup is non-neglectable and should be avoided in this case. Closes #78980 Partially addresses to #77888
martijnvg
added a commit
to martijnvg/elasticsearch
that referenced
this issue
Oct 26, 2021
Backporting elastic#79004 to 7.x branch. In cases when indices, aliases and data streams aren't modified then the indices lookup can be reused. For example in: * The IndexMetadataUpdater#applyChanges(...) method builds a new metadata instance, but only primary term or insync allocations may be updated. No new indices, aliases or data streams are added, so re-building indices lookup is not necessary. * MasterService#patchVersions Additionally the logic that checks when indices lookup can be reused, this logic also checks the hidden and system flags of indices/datastreams. In clusters with many indices the cost of building indices lookup is non-neglectable and should be avoided in this case. Closes elastic#78980 Partially addresses elastic#77888
martijnvg
added a commit
that referenced
this issue
Oct 26, 2021
Backporting #79004 to 7.16 branch. In cases when indices, aliases and data streams aren't modified then the indices lookup can be reused. For example in: * The IndexMetadataUpdater#applyChanges(...) method builds a new metadata instance, but only primary term or insync allocations may be updated. No new indices, aliases or data streams are added, so re-building indices lookup is not necessary. * MasterService#patchVersions Additionally the logic that checks when indices lookup can be reused, this logic also checks the hidden and system flags of indices/datastreams. In clusters with many indices the cost of building indices lookup is non-neglectable and should be avoided in this case. Closes #78980 Partially addresses #77888
lockewritesdocs
pushed a commit
to lockewritesdocs/elasticsearch
that referenced
this issue
Oct 28, 2021
In cases when indices, aliases and data streams aren't modified then the indices lookup can be reused. For example in: * The IndexMetadataUpdater#applyChanges(...) method builds a new metadata instance, but only primary term or insync allocations may be updated. No new indices, aliases or data streams are added, so re-building indices lookup is not necessary. * MasterService#patchVersions Additionally the logic that checks when indices lookup can be reused, this logic also checks the hidden and system flags of indices/datastreams. In clusters with many indices the cost of building indices lookup is non-neglectable and should be avoided in this case. Closes elastic#78980 Partially addresses to elastic#77888
|
Closing this, this has disappeared from profiling entirely in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Today
MasterService#patchVersionscreates a newMetadata$Builder, increments the version, and then callsbuild()which expensively recomputes a bunch of data structures, e.g.Metadata#indicesLookup. This is quite inefficient, we could just copy those data structures from the previousMetadataand save a bunch of work and garbage-generation.This is what it looks like in a flamegraph:
That's only ~7.5% of the total time on this otherwise busy master, but we could still cut this in half.
The text was updated successfully, but these errors were encountered: