-
-
Notifications
You must be signed in to change notification settings - Fork 932
Further optimize cross tile symbol index findMatches
#6641
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 10 commits
efaaea5
c834bd2
1c0d4f2
4600214
6dda944
77a9904
91ad4b2
88ae4e0
0c0f587
392f7d3
c1dc44f
99c7530
b0e7909
1b92449
83c84a8
a98ee11
3404589
b6a37ef
1134558
6574aee
4cf39a2
f4d0304
4beaad2
386288a
578fdd8
20eca82
964916c
4989773
a954ea8
5c2c793
880833a
f729a85
3d218f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,6 +104,8 @@ class TileLayerIndex { | |
| }) { | ||
| const tolerance = this.tileID.canonical.z < newTileID.canonical.z ? 1 : Math.pow(2, this.tileID.canonical.z - newTileID.canonical.z); | ||
|
|
||
| const symbolKeyToScaledCoordinatesToSymbolInstanceMap = new Map<number, Map<number, SymbolInstance[]>>(); | ||
|
|
||
| for (let i = 0; i < symbolInstances.length; i++) { | ||
| const symbolInstance = symbolInstances.get(i); | ||
| if (symbolInstance.crossTileID) { | ||
|
|
@@ -120,25 +122,21 @@ class TileLayerIndex { | |
| const scaledSymbolCoord = this.getScaledCoordinates(symbolInstance, newTileID); | ||
|
|
||
| if (entry.index) { | ||
| // Return any symbol with the same keys whose coordinates are within 1 | ||
| // grid unit. (with a 4px grid, this covers a 12px by 12px area) | ||
| const indexes = entry.index.range( | ||
| scaledSymbolCoord.x - tolerance, | ||
| scaledSymbolCoord.y - tolerance, | ||
| scaledSymbolCoord.x + tolerance, | ||
| scaledSymbolCoord.y + tolerance).sort(); | ||
|
|
||
| for (const i of indexes) { | ||
| const crossTileID = entry.crossTileIDs[i]; | ||
|
|
||
| if (!zoomCrossTileIDs[crossTileID]) { | ||
| // Once we've marked ourselves duplicate against this parent symbol, | ||
| // don't let any other symbols at the same zoom level duplicate against | ||
| // the same parent (see issue #5993) | ||
| zoomCrossTileIDs[crossTileID] = true; | ||
| symbolInstance.crossTileID = crossTileID; | ||
| break; | ||
| // Build a map keyed by common symbol instance keys, which are also | ||
| // used to key indexes. For each symbol index key, build a reverse map | ||
| // of the scaled coordinates back to a set of SymbolInstance that | ||
| // resolve to that coordinate. | ||
| const scaledCoordinateId = scaledSymbolCoord.x << 16 | scaledSymbolCoord.y; | ||
| const existingSymbolKey = symbolKeyToScaledCoordinatesToSymbolInstanceMap.get(symbolInstance.key); | ||
| if (existingSymbolKey) { | ||
| const existingSymbolsAtCoordinate = existingSymbolKey.get(scaledCoordinateId); | ||
| if (existingSymbolsAtCoordinate) { | ||
| existingSymbolsAtCoordinate.push(symbolInstance); | ||
| } else { | ||
| existingSymbolKey.set(scaledCoordinateId, [symbolInstance]); | ||
| } | ||
| } else { | ||
| symbolKeyToScaledCoordinatesToSymbolInstanceMap.set(symbolInstance.key, new Map([[scaledCoordinateId, [symbolInstance]]])); | ||
| } | ||
| } else if (entry.positions) { | ||
| for (let i = 0; i < entry.positions.length; i++) { | ||
|
|
@@ -160,6 +158,52 @@ class TileLayerIndex { | |
| } | ||
| } | ||
| } | ||
|
|
||
| for (const [symbolKey, coordinateMap] of symbolKeyToScaledCoordinatesToSymbolInstanceMap.entries()) { | ||
HarelM marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
HarelM marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| const entry = this._symbolsByKey[symbolKey]; | ||
| if (!entry) { | ||
| // Shouldn't happen, as we only populate keys in this | ||
bradymadden97 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // map above when an entry exists. | ||
| continue; | ||
| } | ||
|
|
||
| if (!entry.index){ | ||
| // Shouldn't happen, as we only populate keys in this | ||
| // map above when the entry has an index built. | ||
| continue; | ||
| } | ||
|
|
||
| for (const [coordinateId, symbolInstancesAtCoordinate] of coordinateMap.entries()) { | ||
| const x = coordinateId >>> 16; | ||
| const y = coordinateId % (1 << 16); | ||
| const indexes = entry.index.range( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to compare this to the previous code and I'm not sure I understand why there will be less calls to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely! So the situation where this is faster is because we're now running a Previously, we would independently run a range query for every single symbol instance, and then match a result from that range query with the symbol instance. A pathological case would be 10,000 symbol instances at the exact same coordinate. Previously that would be 10,000 range queries. Now that would be 1 range query. Based on that algorithm, it appears to never be more range queries than before, but in bad cases it can be significantly less.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this is for x and y that are exactly the same, right? If all the x and y are in slightly different places then it would behave as before, right? |
||
| x - tolerance, | ||
| y - tolerance, | ||
| x + tolerance, | ||
| y + tolerance); | ||
|
|
||
| // Iterate through cross tile entries at this quadrant _and_ | ||
| // symbol instances and pair them up until one of the lists | ||
| // runs out. This is faster than re-running a range query | ||
| // for every symbol instance, as each of these symbol | ||
| // instances already have the same key, and thus can be | ||
| // paired with any entry in the index. | ||
| let i = 0; | ||
| let j = 0; | ||
| while (i < entry.crossTileIDs.length && j < Math.min(indexes.length, symbolInstancesAtCoordinate.length)) { | ||
| const crossTileID = entry.crossTileIDs[i]; | ||
| if (!zoomCrossTileIDs[crossTileID]) { | ||
| // Once we've marked ourselves duplicate against this parent symbol, | ||
| // don't let any other symbols at the same zoom level duplicate against | ||
| // the same parent (see issue #5993) | ||
| zoomCrossTileIDs[crossTileID] = true; | ||
| symbolInstancesAtCoordinate[j].crossTileID = crossTileID; | ||
| j++; | ||
| } | ||
| i++; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| getCrossTileIDsLists() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to use numbers as keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't figure out another way to use the coordinates themselves as keys in a relatively efficient way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's the tileID.key that is usually used, can it be used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This coordinate is within a given tile, so not equivalent to tileID.key I don't think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use strings maybe? I'm terrified of number overflows when I see this kind of code? How worse is the performance if this is string instead of number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to doubly nested map by x/y coordinates if that's better 964916c