Implement setting deduplication via String interning #80493

original-brownbear · 2021-11-08T13:37:28Z

Preamble:
This is a somewhat/admittedly crude solution to #78892 that nevertheless addresses
95%+ of duplicate setting entry memory consumption in large clusters. This has been heavily tested/benchmarked
and no issues have been found. It reduces the heap consumption on every node in the cluster to the same degree
since all of them hold the deserialized settings on heap. In practice this can mean hundreds to GBs of heap savings
on every node in larger clusters. While a more sophisticated solution should eventually be implemented to also
do away with the remaining duplicate data structures the marginal gain of that over this solution is relatively limited
as things work today. Also, this solution is a safe and cheap win that we could potentially even backport to 7.16.1
to bring down memory consumption of common index types there.

The remaining duplicate structures (lists of all the same strings) are
comparatively cheap in their heap consumption.
In heavy benchmarking for #77466 no runtime impact of adding this extra step
to setting creation has been found despite pushing setting creation harder
than is expected in real-world usage (part of the low relative impact here is
the fact that populating a tree-map is quite expensive to begin with so adding
the string interning which is fast via the CHM cache doesn't add much overhead).
On the other hand, the heap use impact for use-cases that come with a large number
of duplicate settings (many similar indices) is significant. As an example,
10k Audit Beat indices consume about 500M of heap for duplicate settings data structures
without this change. This change brings the heap consumption from duplicate settings down to
O(1M) on every node in the cluster.

Relates and addresses most of #78892
Relates #77466

This is a somewhat crude solution to elastic#78892 that addresses 95%+ of duplicate setting entry memory consumption in large clusters. The remaining duplicate structures (lists of all the same strings) are comparatively cheap in their heap consumption. In heavy benchmarking for elastic#77466 no runtime impact of adding this extra step to setting creation has been found despite pushing setting creation harder than is expected in real-world usage (part of the low relative impact here is the fact that populating a tree-map is quite expensive to begin with so adding the string interning which is fast via the CHM cache doesn't add much overhead). On the other hand, the heap use impact for use-cases that come with a large number of duplicate settings (many similar indices) is significant. As an example, 10k AuditBeat indices consume about 500M of heap for duplicate settings data structures without this change. This cahnge brings the heap consumption from duplicate settings down to O(1M) on every node in the cluster. Relates and addresses most of elastic#78892 Relates elastic#77466

elasticmachine · 2021-11-08T13:37:32Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

original-brownbear · 2021-11-08T13:39:00Z

server/src/main/java/org/elasticsearch/common/util/StringLiteralDeduplicator.java

@@ -20,8 +18,6 @@
 */
 public final class StringLiteralDeduplicator {

-    private static final Logger logger = LogManager.getLogger(StringLiteralDeduplicator.class);


We use this thing before the logging is fully initialized now in some cases leading to the same issue we resolve by indirection in org.elasticsearch.common.settings.Settings.DeprecationLoggerHolder. I don't think the logging here is valuable enough to go for any kind of sophisticated solution to keep it around so I just dropped it.

original-brownbear · 2021-11-08T13:44:41Z

Also relates #80348

grcevski

LGTM!

henningandersen

I would like to have a test added that verifies (and ensures we preserve) the sharing of strings. Perhaps an integ-test, with two nodes and two indices, verifying that a few keys/values from the index-settings are shared between the indices on both nodes. Otherwise this looks good to me.

henningandersen · 2021-11-09T06:19:46Z

server/src/main/java/org/elasticsearch/common/settings/Settings.java

@@ -99,7 +100,43 @@ private static Settings of(Map<String, Object> settings, SecureSettings secureSe

    private Settings(Map<String, Object> settings, SecureSettings secureSettings) {
        // we use a sorted map for consistent serialization when using getAsMap()
-        this.settings = Collections.unmodifiableNavigableMap(new TreeMap<>(settings));


I think I would prefer this in Builder.build(). Seems more logical and also, AFAICS, that is the only path we need to cover.

I found it a little easier to reason about to keep it here, so we are always guaranteed to get a fully immutable structure now (also for lists etc.).
Also technically org.elasticsearch.common.settings.Settings#getByPrefix
which uses Settings.of( profits from this change as well so maybe just keep it here in the constructor to keep things simple and have this apply in all cases? We could look into some optimizations in a follow-up maybe, but generally as discussed elsewhere setting setup isn't a hot path issue.

I mainly found it illogical that we do not fully build the parameters for the constructor in the build method like we normally do. I would still prefer it the other way around, but can accept it residing here for now.

I am not sure I found a place where getByPrefix needs this, they all look like "get and throw away" style lookups, perhaps you have a specific case. Even if there are such cases, it seems unnecessary to go through the lists once more when all we need is interned keys in that case.

henningandersen · 2021-11-09T06:24:15Z

server/src/main/java/org/elasticsearch/common/settings/Settings.java

+                    }
+                }
+                if (notInternedOffset >= 0) {
+                    final String[] internedArr = new String[listSize];


I wonder if we should not just copy the list every time, interning it, rather than this optimization? The only thing we are saving is this array, but we still copy the list anyway so hardly seems worth the complexity added here.

The reason I made it this way actually wasn't to save work in the constructor. I was going for re-using the full list object if all its contents are interned already. I had some concrete and short ideas for exploiting that for another round of savings on the settings and I suppose it already does do some saving today.

I would prefer to omit this then. As mentioned above, this is not on the hot path. And if we want to optimize it further, it seems more intuitive to do so in the builder, since we could intern the strings in the list in putList and assume all lists are interned (and immutable) always (and likewise for keys).

If follow-up work requires this we can re-add it, but I find this too much of half an optimization in the current state.

Sounds good, always copying for now :)

…ntern

original-brownbear · 2021-11-09T20:13:15Z

Thanks @henningandersen I added a test that checks that we actually intern all these strings but had some reservations on the code changes. Let me know what you think :)

henningandersen

LGTM, thanks for adding the test.

henningandersen · 2021-11-10T08:07:55Z

server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java

+        assertEqualsAndStringsInterned(queryFieldsSetting, updatedIndex1SettingsMaster);
+        assertEqualsAndStringsInterned(queryFieldsSetting, updatedIndex1SettingsData);
+        assertEqualsAndStringsInterned(queryFieldsSetting, clusterServiceMaster.state().metadata().index(index2).getSettings());
+        assertEqualsAndStringsInterned(queryFieldsSetting, clusterServiceData.state().metadata().index(index2).getSettings());


Can we also assert that a key is interned? Could just check that the key on index1 is the same as on index2.

Sure, I actually just made it assert that all of the keyset is interned strings on these settings now. That was easier (hard to get a reference to a setting key directly) and stricter :)

henningandersen · 2021-11-10T08:09:58Z

server/src/main/java/org/elasticsearch/common/settings/Settings.java

+                    }
+                }
+                if (notInternedOffset >= 0) {
+                    final String[] internedArr = new String[listSize];


I would prefer to omit this then. As mentioned above, this is not on the hot path. And if we want to optimize it further, it seems more intuitive to do so in the builder, since we could intern the strings in the list in putList and assume all lists are interned (and immutable) always (and likewise for keys).

If follow-up work requires this we can re-add it, but I find this too much of half an optimization in the current state.

henningandersen · 2021-11-10T08:20:18Z

server/src/main/java/org/elasticsearch/common/settings/Settings.java

@@ -99,7 +100,43 @@ private static Settings of(Map<String, Object> settings, SecureSettings secureSe

    private Settings(Map<String, Object> settings, SecureSettings secureSettings) {
        // we use a sorted map for consistent serialization when using getAsMap()
-        this.settings = Collections.unmodifiableNavigableMap(new TreeMap<>(settings));


I mainly found it illogical that we do not fully build the parameters for the constructor in the build method like we normally do. I would still prefer it the other way around, but can accept it residing here for now.

I am not sure I found a place where getByPrefix needs this, they all look like "get and throw away" style lookups, perhaps you have a specific case. Even if there are such cases, it seems unnecessary to go through the lists once more when all we need is interned keys in that case.

…ntern

original-brownbear · 2021-11-10T10:47:42Z

Thanks Nikolai + Henning!

This is a somewhat crude solution to elastic#78892 that addresses 95%+ of duplicate setting entry memory consumption in large clusters. The remaining duplicate structures (lists of all the same strings) are comparatively cheap in their heap consumption. In heavy benchmarking for elastic#77466 no runtime impact of adding this extra step to setting creation has been found despite pushing setting creation harder than is expected in real-world usage (part of the low relative impact here is the fact that populating a tree-map is quite expensive to begin with so adding the string interning which is fast via the CHM cache doesn't add much overhead). On the other hand, the heap use impact for use-cases that come with a large number of duplicate settings (many similar indices) is significant. As an example, 10k AuditBeat indices consume about 500M of heap for duplicate settings data structures without this change. This cahnge brings the heap consumption from duplicate settings down to O(1M) on every node in the cluster. Relates and addresses most of elastic#78892 Relates elastic#77466

This is a somewhat crude solution to #78892 that addresses 95%+ of duplicate setting entry memory consumption in large clusters. The remaining duplicate structures (lists of all the same strings) are comparatively cheap in their heap consumption. In heavy benchmarking for #77466 no runtime impact of adding this extra step to setting creation has been found despite pushing setting creation harder than is expected in real-world usage (part of the low relative impact here is the fact that populating a tree-map is quite expensive to begin with so adding the string interning which is fast via the CHM cache doesn't add much overhead). On the other hand, the heap use impact for use-cases that come with a large number of duplicate settings (many similar indices) is significant. As an example, 10k AuditBeat indices consume about 500M of heap for duplicate settings data structures without this change. This cahnge brings the heap consumption from duplicate settings down to O(1M) on every node in the cluster. Relates and addresses most of #78892 Relates #77466

This is a somewhat crude solution to elastic#78892 that addresses 95%+ of duplicate setting entry memory consumption in large clusters. The remaining duplicate structures (lists of all the same strings) are comparatively cheap in their heap consumption. In heavy benchmarking for elastic#77466 no runtime impact of adding this extra step to setting creation has been found despite pushing setting creation harder than is expected in real-world usage (part of the low relative impact here is the fact that populating a tree-map is quite expensive to begin with so adding the string interning which is fast via the CHM cache doesn't add much overhead). On the other hand, the heap use impact for use-cases that come with a large number of duplicate settings (many similar indices) is significant. As an example, 10k AuditBeat indices consume about 500M of heap for duplicate settings data structures without this change. This cahnge brings the heap consumption from duplicate settings down to O(1M) on every node in the cluster. Relates and addresses most of elastic#78892 Relates elastic#77466

This is a somewhat crude solution to #78892 that addresses 95%+ of duplicate setting entry memory consumption in large clusters. The remaining duplicate structures (lists of all the same strings) are comparatively cheap in their heap consumption. In heavy benchmarking for #77466 no runtime impact of adding this extra step to setting creation has been found despite pushing setting creation harder than is expected in real-world usage (part of the low relative impact here is the fact that populating a tree-map is quite expensive to begin with so adding the string interning which is fast via the CHM cache doesn't add much overhead). On the other hand, the heap use impact for use-cases that come with a large number of duplicate settings (many similar indices) is significant. As an example, 10k AuditBeat indices consume about 500M of heap for duplicate settings data structures without this change. This cahnge brings the heap consumption from duplicate settings down to O(1M) on every node in the cluster. Relates and addresses most of #78892 Relates #77466

original-brownbear added >enhancement :Core/Infra/Settings Settings infrastructure and APIs v8.0.0 v8.1.0 labels Nov 8, 2021

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Nov 8, 2021

original-brownbear commented Nov 8, 2021

View reviewed changes

grcevski approved these changes Nov 8, 2021

View reviewed changes

henningandersen reviewed Nov 9, 2021

View reviewed changes

original-brownbear added 3 commits November 9, 2021 11:13

Merge remote-tracking branch 'elastic/master' into setting-dedup-by-i…

17c123f

…ntern

Merge remote-tracking branch 'elastic/master' into setting-dedup-by-i…

bd9004e

…ntern

test

a5d2520

original-brownbear requested a review from henningandersen November 9, 2021 20:12

henningandersen approved these changes Nov 10, 2021

View reviewed changes

original-brownbear added 2 commits November 10, 2021 10:48

Merge remote-tracking branch 'elastic/master' into setting-dedup-by-i…

1cfb2b9

…ntern

intern all the things

6a81b69

original-brownbear merged commit ab2f5cc into elastic:master Nov 10, 2021

original-brownbear deleted the setting-dedup-by-intern branch November 10, 2021 10:47

original-brownbear mentioned this pull request Nov 10, 2021

Fix Large Shard Count Scalability Issues #77466

Open

97 tasks

original-brownbear mentioned this pull request Nov 10, 2021

Implement Setting Deduplication via String Interning (#80493) #80590

Merged

original-brownbear added the v7.17.0 label Jan 17, 2022

original-brownbear mentioned this pull request Jan 17, 2022

Implement Setting Deduplication via String Interning (#80493) #82659

Merged

albertzaharovits changed the title ~~Implement Setting Deduplication via String Interning~~ Implement setting deduplication via String interning Jan 25, 2022

original-brownbear mentioned this pull request Jan 27, 2022

A Node Joining a Cluster with a Large State Receives the Full Uncompressed State in a ValidateJoinRequest #83204

Closed

pugnascotia added v8.0.0-rc2 and removed v8.0.0 labels Feb 1, 2022

original-brownbear restored the setting-dedup-by-intern branch April 18, 2023 20:56

Implement setting deduplication via String interning #80493

Implement setting deduplication via String interning #80493

Uh oh!

Conversation

original-brownbear commented Nov 8, 2021

Uh oh!

elasticmachine commented Nov 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Nov 8, 2021

Uh oh!

grcevski left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Nov 9, 2021

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Nov 10, 2021

Uh oh!

Uh oh!