Script: Metadata validateMetadata optimization #93333
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The safety guarantees are essentially the same, but in practice this prevents us from wrapping an Map.of(...) -generated map (i.e. an ImmutableMap) in another layer of unmodifiability (i.e. an UnmodifiableMap).
We only call it from within this package or in subclasses via calls to `super(...)`.
joegallo
added
>enhancement
:Core/Infra/Scripting
|
Hi @joegallo, I've created a changelog YAML for you. |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
thecoop
reviewed
Jan 30, 2023
stu-elastic
reviewed
Jan 30, 2023
| /** | ||
| * Check that all metadata map contains only valid metadata and no extraneous keys | ||
| */ | ||
| protected void validateMetadata() { | ||
| int numMetadata = 0; | ||
| for (Map.Entry<String, FieldProperty<?>> entry : properties.entrySet()) { | ||
| String key = entry.getKey(); | ||
| if (map.containsKey(key)) { | ||
| Object value = map.getOrDefault(key, NOT_FOUND); | ||
| if (value != NOT_FOUND) { |
There was a problem hiding this comment.
Can you flip this condition? I know why it's written this way atm but not NOT_FOUND doesn't read nicely.
There was a problem hiding this comment.
Oh, yeah, it's way better as the reverse. Thanks! fdd1f27
stu-elastic
reviewed
Jan 30, 2023
| numMetadata++; | ||
| // check whether it's permissible to have the value for the property |
There was a problem hiding this comment.
This comment can be dropped, the other is sufficient. Consider adding a comment of the form "getOrDefault is faster than hasKey + get".
stu-elastic
reviewed
Jan 30, 2023
| // we can't tell the compiler that properties must be a java.util.ImmutableCollections.AbstractImmutableMap, but | ||
| // we can use this copyOf + assert to verify that at runtime. | ||
| this.properties = Map.copyOf(properties); | ||
| assert this.properties == properties : "properties map must be constructed via Map.of(...) or Map.copyOf(...)"; |
There was a problem hiding this comment.
Can you add a test for this assert?
There was a problem hiding this comment.
Gah, yeah, I fall upon my sword. 0c97aa3
stu-elastic
reviewed
Jan 30, 2023
thecoop
approved these changes
Jan 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Map.copyOf
The first load-bearing bit here is the change from
this.properties = Collections.unmodifiableMap(properties);tothis.properties = Map.copyOf(properties);. Since all non-test callers pass in apropertiesmap that was constructed via a call toMap.of, it's already an "immutable map" andMap.copyOfends up being a no-op.The precise details of this are very annoying, but if you trace
Map.copyOfandCollections.unmodifiableMapthrough, both do have shortcutting behavior for multiple calls to themselves, but they are not mutually shortcutting -- so even though all our non-test properties maps are constructed as immutable via a call toMap.of, they were still wrapped in another layer on unmodifiability via the call toCollections.unmodifiableMap. On its own that's not so bad, it's just an allocation, but the wrapping also extends to another immutable wrapper being generated around each individualEntrywhen you loop over one of these maps viaentrySet, which we do on construction for all of these maps in the form ofvalidateMetadata().That is, prior to this PR, we were at O(N*M) allocations here -- N on the number of Metadata being constructed, and M on the number of entries in their associated properties. After this PR there are no additional allocations, just N no-oping method calls.
getOrDefault
The second important change is that rather than calling
.containsKey(key)and then immediately calling.get(key)after that, we can callgetOrDefault(key, NOT_FOUND)just once and avoid an extra hashmap lookup. It's only a factor of 2 (so not actually speedup in terms of big O, right?), but it is still a small speedup in reality.Overall, this changes the picture from this flamegraph before:
To this flamegraph after:
I've normalized the
Matched: x.xx%to be as compared to all of the time spent in the IngestService, so these changes together are shaving 0.5% off our overall ingest time. Not an enormous win, but the changes themselves are pretty tidy and self-contained, so I think this is a nice small speedup as low-hanging fruit.