Speed up Data Tier Allocation Decider + Related Settings #78608
Conversation
* Short circuit some string splitting loops where possible. * Fix very inefficient setting methods that would loop the properties on every lookup and/or bulid the default value string on every default lookup. * Use plain setting get instead of the setings instance get for very hot setting * Simplify hot path for `getAsBoolean`
original-brownbear
added
>non-issue
:Core/Infra/Settings
elasticmachine
added
Team:Core/Infra
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
…-alloc-decider-even-more
There was a problem hiding this comment.
The properties change looks good.
For looking up the boolean directly on the settings object, this is the opposite direction that we have been trying to go. It is my hope that we could eventually get rid of all the getters directly on settings objects, or at least move them into their appropriate setting instance type.
Why are settings a hot path in the first place? I think anytime we have settings on the hot path we should move the value out to a member. Settings don’t change often, or in some cases it is static for the life of an index.
| @@ -25,7 +25,7 @@ | |||
| private SearchableSnapshotsSettings() {} | |||
|
|
|||
| public static boolean isSearchableSnapshotStore(Settings indexSettings) { | |||
| return SEARCHABLE_SNAPSHOT_STORE_TYPE.equals(INDEX_STORE_TYPE_SETTING.get(indexSettings)); | |||
| return SEARCHABLE_SNAPSHOT_STORE_TYPE.equals(indexSettings.get(INDEX_STORE_TYPE_SETTING.getKey())); | |||
There was a problem hiding this comment.
If this is hot then we should move it to be directly represented on the IndexSettings?
There was a problem hiding this comment.
Yea let's do it :) I'll back this part out into a separate PR in a bit then and just leave the obvious optimizations here.
| @@ -321,7 +321,21 @@ public boolean hasValue(String key) { | |||
| * returns the default value provided. | |||
| */ | |||
| public Boolean getAsBoolean(String setting, Boolean defaultValue) { | |||
| return Booleans.parseBoolean(get(setting), defaultValue); | |||
| final Object found = settings.get(setting); | |||
| if (found == null) { | |||
There was a problem hiding this comment.
Couldn’t this optimizations exist inside Booleans.parseBoolean?
original-brownbear
removed
>non-issue
:Core/Infra/Settings
|
closing as this has been resolved by a different approach now |
on every lookup and/or build the default value string on every default lookup.
getAsBooleanrelates #77466
addressed part of #77974 by making the boolean setting much cheaper