fix(polygonize): Migrate edge storage from Array to Map for performance - #3081
Merged
Conversation
mfedderly
commented
Jun 26, 2026
Comment on lines
+118
to
+132
| // add the forward edge | ||
| let toMap = this.edges.get(from); | ||
| if (toMap == null) { | ||
| toMap = new Map(); | ||
| this.edges.set(from, toMap); | ||
| } | ||
| toMap.set(to, edge); | ||
|
|
||
| // add the symmetric edge | ||
| let symToMap = this.edges.get(to); | ||
| if (symToMap == null) { | ||
| symToMap = new Map(); | ||
| this.edges.set(to, symToMap); | ||
| } | ||
| symToMap.set(from, symetricEdge); |
Collaborator
Author
There was a problem hiding this comment.
This block actually implicitly deduplicates edges as they're inserted. We'll create a new edge and clobber the previous edge with the same (to, from) Nodes.
Collaborator
Author
There was a problem hiding this comment.
After a bit more research, the Edge constructor modifies the passed-in Node objects. That means that merely creating an Edge will still cause the bug that #3045 set out to address.
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.
A follow on update to #3074, working towards #3045
This migrates edge storage to
Map<Node, Map<Node, Edge>>which lets us look up an edge directly by its nodes.Maps are slightly slower to iterate than arrays, so this is a bit of a regression from the performance achieved in #3074, but is faster than the original version before that PR. Our test data only has one complex input geometry, and that performance is about doubled by this PR vs the #3074 version. We're trading slight slowness on the simple cases that are already quite fast for much better performance in the cases that are actually slow.
Using a Map here instead of a Set sets us up nicely for the last PR which will prevent inserting duplicate edges. We need the map to look up any existing edges directly. I also noticed that the duplicate accounting for Nodes isn't really needed, we can get away with just iterating the index version. This is a slight speedup in all cases by avoiding the double storage.
The test outputs are visually identical, but changed the ordering slightly because the iteration order changes (they were previously ordered entirely by insertion order, but now they are ordered by
toinsertion order and thenfrom)