Use foreachEntry and avoid repeated Map.updated in invert - #84
Open
cheeseng wants to merge 1 commit into
Open
Conversation
The first loop consumed `this` (a Map) via a manual iterator/while loop, destructuring each (K, V) tuple by hand. Switch to foreachEntry, which for Map/HashMap-backed types walks the internal table directly and passes key/value as separate arguments — no tuple is built or unpacked per entry. The second loop rebuilt the result via repeated result.updated(...) calls on an immutable.Map, which reallocates trie nodes on every call and discards the previous version each time. Replace it with a single immutable.Map.newBuilder, staged once via the existing foreachEntry pass over m and materialized once via b.result() — same fix already applied in groupFlatMap. No sizeHint added to m or b: the number of distinct inverted keys and how the original entries distribute across them is data-dependent and unknown before the first pass, so no reliable size estimate exists in advance. No behavior changes.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
invert had two separate inefficiencies:
The first loop traversed
thisvia a manualval it = iterator; while (it.hasNext) { val kv = it.next(); ... },destructuring kv._1/kv._2 by hand. Switched to foreachEntry, which
for Map/HashMap-backed types walks the internal table directly and
invokes the callback with key and value as separate arguments,
avoiding the tuple allocation/unpacking the manual iterator version
paid for on every entry.
The second loop rebuilt the result via
result = result.updated(v, bldr.result())inside m.foreachEntry.Each .updated call on an immutable.Map reallocates trie nodes along
the path to the changed key and discards the previous map version
as garbage — the same anti-pattern already fixed in groupFlatMap.
Replaced it with a single immutable.Map.newBuilder, populated via
the same foreachEntry pass and materialized once via b.result().
No sizeHint added to either builder: the number of distinct inverted
keys, and how source entries distribute across them, is
data-dependent and unknowable before the first pass completes — a
hint sized against
thiswould risk over-allocating whenever manykeys invert to the same value, which is a common case for this method.
No behavior changes.