Intern IndexFieldCapabilities Type String on Read #76405
Conversation
In case of handling a large number of these messages, i.e. when fetching field caps for many indices (and/or those indices contain lots of fields) the type string is repeated many times over. As these strings are already interned because they are constants, taking the performance hit of interning them on deserialization seems a reasonable trade-off for the benefit of saving a non-trivial amount of memory for large clusters as well as speeding up `org.elasticsearch.action.fieldcaps.TransportFieldCapabilitiesAction#merge` which uses these strings in map lookup and will run significantly faster with interned strings instead of fresh strings that do not have their hash values cached yet.
original-brownbear
added
:Data Management/Indices APIs
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-core-features (Team:Core/Features) |
| @@ -50,7 +50,7 @@ | |||
|
|
|||
| IndexFieldCapabilities(StreamInput in) throws IOException { | |||
| this.name = in.readString(); | |||
| this.type = in.readString(); | |||
| this.type = in.readString().intern(); | |||
There was a problem hiding this comment.
I believe String.intern is usually not recommended because you have no control over the size of the string pool. It used to be pretty bad before Java 8 where the string pool was finally moved to the heap from PermGen. I think a better approach would be to use a ConcurrentMap with Weak/SoftReferences or an actual Cache if it's available.
There was a problem hiding this comment.
https://github.com/FasterXML/jackson-databind/blob/2.13/src/main/java/com/fasterxml/jackson/databind/util/LRUMap.java#L24
For example, the approach which is used in Jackson for caching
There was a problem hiding this comment.
@arteam True true, in this one instance I figured I could get away with it because we know for a fact that these strings will be in the pool (these are field types and the field mapper objects all have them as concrete strings in the source).
I could certainly see the point of using a static constant CHM and putting the interned strings in therefor performance which will probably be quite a bit faster even in recent JDK. It's just extra code and maybe a bit of a risk in case the type ever stops coming from a constant pool of strings.
There was a problem hiding this comment.
@arteam I made this a little more "high-tech" now by putting a CHM in front of intern :) Should be way faster now with the same result as before. Let me know what you think :)
…-caps-type-on-deserialization
server/src/main/java/org/elasticsearch/common/util/StringLiteralDeduplicator.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/StringLiteralDeduplicator.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/StringLiteralDeduplicator.java
Show resolved
Hide resolved
…-caps-type-on-deserialization
jtibshirani
added
the
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
|
|
||
| private static final Logger logger = LogManager.getLogger(StringLiteralDeduplicator.class); | ||
|
|
||
| private static final int MAX_SIZE = 1000; |
There was a problem hiding this comment.
This will start thrashing once we exceed 1000 strings. I worry about the reuse of this INSTANCE for other purposes. Can we instead make a single instance that is used solely for the field capabilities type field by making a private static instance in the IndexFieldCapabilities class? If other needs arise for this deduplicator, those would thereby be separated completely.
There was a problem hiding this comment.
++ made it a private static cache for now just limited to this class
| final String interned = string.intern(); | ||
| if (map.size() > MAX_SIZE) { | ||
| boolean cleared = false; | ||
| synchronized (this) { |
There was a problem hiding this comment.
I think exceeding the MAX_SIZE is unexpected. I would advocate not synchronizing in that case, since this runs on the transport thread and simply clear the map in all threads that enter this.
There was a problem hiding this comment.
Sure if we only use this in a limited case ++ to that, simplified as requested :)
…-caps-type-on-deserialization
|
Sorry for jumping in late. I was wondering if you had rough numbers showing how much this helps in terms of speed + memory? I am curious because it does add a little complexity, and imagine there are other big contributors to speed/ memory. |
|
No worries @jtibshirani .
Speed I'm having a hard time predicting. The only thing I have to go by here was a user running into trouble in the last step of this transport action where using the type a map key got slow on hashing it (because it's a different string over and over I assume combined with the fact that its internal bytes might not be in the CPU cache after waiting for other responses for a longer period of time) => I could see a bit of a gain here from just keying by the same type string instances every time when merging and in the specific case it would be a measurable speedup I think. Memory is more straight forward though I think. If you take a field type string like "_routing" that is ~50b for each string instance. That quickly translates into a couple of MB saved and I've seen nodes under heavy load from these requests have hundreds of MB of duplicate type strings on heap in real-world dumps. |
|
I'm merging this one now. We've identified additional spots where deduplicating these very same strings will be helpful so the added complexity will soon be reusable :) Thanks everyone! |
original-brownbear
deleted the
intern-index-field-caps-type-on-deserialization
branch
In case of handling a large number of these messages, i.e. when fetching field caps for many indices (and/or those indices contain lots of fields) the type string is repeated many times over. As these strings are already interned because they are constants, taking the performance hit of interning them on deserialization seems a reasonable trade-off for the benefit of saving a non-trivial amount of memory for large clusters as well as speeding up `org.elasticsearch.action.fieldcaps.TransportFieldCapabilitiesAction#merge` which uses these strings in map lookup and will run significantly faster with interned strings instead of fresh strings that do not have their hash values cached yet.
In case of handling a large number of these messages, i.e. when fetching field caps for many indices (and/or those indices contain lots of fields) the type string is repeated many times over. As these strings are already interned because they are constants, taking the performance hit of interning them on deserialization seems a reasonable trade-off for the benefit of saving a non-trivial amount of memory for large clusters as well as speeding up `org.elasticsearch.action.fieldcaps.TransportFieldCapabilitiesAction#merge` which uses these strings in map lookup and will run significantly faster with interned strings instead of fresh strings that do not have their hash values cached yet.
original-brownbear
restored the
intern-index-field-caps-type-on-deserialization
branch
Kind of a brute force approach to the problem but this should improve a few reported instances of slow+memory intesive field caps at no real risk IMO:
In case of handling a large number of these messages, i.e. when fetching field caps
for many indices (and/or those indices contain lots of fields) the type string is repeated
many times over. As these strings are already interned because they are constants, taking
the performance hit of interning them on deserialization seems a reasonable trade-off
for the benefit of saving a non-trivial amount of memory for large clusters as well as
speeding up
org.elasticsearch.action.fieldcaps.TransportFieldCapabilitiesAction#mergewhich uses these strings in map lookup and will run significantly faster with interned strings
instead of fresh strings that do not have their hash values cached yet.