Further optimize cross tile symbol index `findMatches` by bradymadden97 · Pull Request #6641 · maplibre/maplibre-gl-js

bradymadden97 · 2025-10-31T18:23:55Z

Issue

When zooming in or out over a source that has a large number of symbols with the same label (or no label), maplibre will hit an extremely slow path trying to find and pair symbols across tiles (for fading behavior). This will result in noticable stalls and frame drops while zooming. This is also documented in the linked issues below.

Demo of issue

Screen.Recording.2025-10-31.at.3.10.30.PM.1.mov

(this is using https://jsbin.com/girobohihu/1/edit?html,console,output which is the simplest repro I could create. I've seen much worse stalls in production and the linked issues have some examples as well). The TLDR of the issue is:

Many symbols near eachother
All with same label or no label at all
Zoom in/ out quickly
See stallling

Background

Essentially, as I understand it, what findMatches is doing is iterating through all the symbols on the current tile, and all the symbols on a future tile (at a different zoom level, depending on if we're zooming in or out) and trying to pair as many symbols from the starting tile and the destination tile as possible. When symbols are "paired", the symbol on the destination tile will not "fade in". If the symbols did always "fade in" it gives a weird experience while zooming. The video below mocks what it would look like if we never paired any symbols while zooming:

Screen.Recording.2025-10-31.at.2.17.04.PM.1.mov

You can see the symbol flash every time we enter a new zoom level, as it's reloaded and unpaired/transitioned from the previous zoom level.

Compare that to the current state:

Screen.Recording.2025-10-31.at.2.18.43.PM.1.mov

So I think we established the purpose of the code, now let's talk about the pathologically slow case. The slow case happens when we have a bunch of symbols on the origin tile that need to be matched with a bunch of symbols on the destination tile. This happens when all symbols have the same label, as the label is what is used to match symbols. However, unlabeled symbols are also essentially treated as being "the same label" for this matching purpose, so a simple pathologically slow case can occur any time we just have a bunch of symbols with no label in the same tile.

We tried to optimize this exact case in #1755 by building a spatial index per unique symbol key (aka label, or no label) (source). But then we ultimately re-run the range query - which is the slow path as highlighted in some of the linked issues - for every single symbol instance (of which there are many) [source].

There was a PR to attempt to exit early from these range queries after we've found 1 result (see here), but I actually don't think this solution would solve this optimally. You wouldn't necessarily want to only fetch 1 result from your range query, as it's possible that first result would have already been paired with a different symbol instance at the same zoom level with the same key. So then the question becomes: "How many do you fetch?", and there is no answer that can be correct besides "All of them", which is what we're currently doing, and is currently slow.

Changes in this PR

That brings us to the changes in this PR. The idea here is to find a way to avoid running the slow range query N times, where N is the number of symbol instances with the same symbol key. We want to run the range query for a given symbol key once, and then pair as many of the results with as many symbol instances as possible.

Effectively the pseduo code changes from:

// assuming all symbols have the same key

for symbol in symbols:
  let entries = index.range(symbol.location);
  for entry in entries:
    if entry.isNotAlreadyPaired:
      symbol.pair = entry
      break;

to

// assuming all symbols have the same key

for location in locationToSymbols:
  let entries = index.range(location);
  loop until entries or location.symbols is exhausted:
    location.symbols[index] = entries[index];
    index++

Before vs. After

Running performance profiles on this example https://jsbin.com/girobohihu/1/edit?html,console,output

The code to run locally (which is where the screenshots themselves came from) is here:

pnpm run dev:repro slow-cross-tile-symbol-index

Manual Regression Test

I performed a manual regression test with labels to confirm our matching behavior still works with this new algorithm. You can see 52 does not fade in/out which implies it is being matched across tiles:

Screen.Recording.2025-10-31.at.3.15.05.PM.2.mov

Benchmark

Wrote a small benchmark to demonstrate the speedup. There is a third comparison, to an internal fork we have that had some additional optimizations beyond what is currently in main, but this improvement exceeds the improvement on that fork.

This was ran last month, against 5.7, but there have been no changes to this file since then, besides #6635 which was just a rename, so they should still be valid.

let X = 10 // 100, 1000, 2000, 3000, 10_000, etc.
export class CrossTileSymbolIndexX extends Benchmark {

    bench() {
        const index = new Index();

        const mainID = new OverscaledTileID(6, 0, 6, 8, 8);
        const childID = new OverscaledTileID(7, 0, 7, 16, 16);

        const mainInstances: any[] = [];
        const childInstances: any[] = [];

        for (let i = 0; i < X; i++) {
            mainInstances.push(makeSymbolInstance(0, 0, ''));
            childInstances.push(makeSymbolInstance(0, 0, ''));
        }
        const mainTile = makeTile(mainID, mainInstances);
        const childTile = makeTile(childID, childInstances);
        index.addLayer(styleLayer as any, [mainTile], 0);
        index.addLayer(styleLayer as any, [childTile], 0);
    }
};

Iterations	Maplibre 5.7	Maplibre 5.7 + PR	Speedup
1k			5.7: 233x Pal: 20x
2k		Not run, see 3,000	See 3,000
3k			5.7: 661x Pal: 55x
10k	Times out / never finishes		5.7: n/a Pal: 111x

Related issues

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Post benchmark scores.
Add an entry to CHANGELOG.md under the ## main section.

codecov · 2025-10-31T18:35:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.47%. Comparing base (9e6a963) to head (7f28e73).
⚠️ Report is 309 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6641      +/-   ##
==========================================
+ Coverage   92.46%   92.47%   +0.01%     
==========================================
  Files         288      288              
  Lines       23856    23890      +34     
  Branches     5063     5071       +8     
==========================================
+ Hits        22058    22092      +34     
  Misses       1798     1798

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

HarelM · 2025-11-01T14:19:50Z

Thanks for taking the time to solve this!
Is there a chance to write a test to cover this scenario or this is a "proper" refactoring so no changes to logic besides performance optimization?

HarelM · 2025-11-01T14:29:01Z

The explanation in the PR is just amazing! Thanks for all the work on this, it really helps reviewing this change.

…//github.com/bradymadden97/maplibre-gl-js into bmadden/cross-tile-symbol-index-optimization

… bmadden/cross-tile-symbol-index-optimization

bradymadden97 · 2025-11-01T20:34:41Z

Is there a chance to write a test to cover this scenario or this is a "proper" refactoring so no changes to logic besides performance optimization?

Found a small bug in the logic that the updates to the test caught. Fixed in the latest version. Test also passes on main. I think this update to the test should cover the refactor logic.

HarelM · 2025-11-03T11:57:19Z

Can you add a changelog entry please?

HarelM · 2025-11-03T12:11:16Z

I've added a few minor comments. the map of maps is a bit confusing to understand so I would recommend adding some docs and export some part of the method to smaller methods with good documentation.
Other than that, this looks good!
Thanks for all the work on this!

…//github.com/bradymadden97/maplibre-gl-js into bmadden/cross-tile-symbol-index-optimization

HarelM · 2025-11-10T20:48:44Z

+
+        // does not match Windsor because of different key
+        const windsorInstance = childInstances.find(i => i.key === 'Windsor');
+        expect(windsorInstance.crossTileID).toEqual(131);


This test has too many expect calls, I would recommend splitting this test and use beforeEach to arrange all the relevant information if it is similar between the tests.
The comments above each paragraph is a good indication that this needs to be split and the comment should probably be the test name.

Was copying the structure of the section of tests above, which do have interleaved expects and do not use beforeEach. I would suggest the structures remain the same actually, in my opinion. Do you agree / would you prefer me to include beforeEach refactor for the existing tests in the other decribe block as well?

I think it would be better to make the file similar to other files and also make sure it is consistent within the same file, meaning change to use beforeEach etc.
Having said that, I don't want to force you to rewrite the entire file, so at least change the tests you added, it would be great if you would change the other tests as well, but I would understand if you decide not to.

After re-looking at it, most of these tests require a good bit of set up to get into the state we ultimately are testing. I'm not sure it makes sense / is easily possible to split them up, and I don't see a lot of shared setup across tests that can be pulled into a beforeEach.

If you wouldn't mind could you take another look at a few of these and see if you think otherwise?

This test specific has a lot of steps and logic.
I would recommend move the initial 20-30 lines into a beforeEach and then test only the part according to each test.
This would mean that you would split this test every time you wrote a comment like:
matched Detroit children tiles from parent for example.

This would make the definition of the test more readable and avoid the need to have out of sync comments inside the test.

If this is not possible to split this test up, then a re-write of this test in needed as it just has too many logic and tests "too much".

I hope this is clearer now.

HarelM · 2025-11-10T20:56:56Z

-                    }
+            const xMap = instancesByScaledCoordinate.get(scaledSymbolCoord.x);
+            if (xMap) {
+                const yMap = xMap.get(scaledSymbolCoord.y);


is yMap a map or an array? I don't see method related to Map when using yMap, am I missing anything?

Does this rename improve it or not? 5c2c793

It's a doubly nested map (since I need to group instances by their x,y coordinates). Happy to change to other suggestions, or clarify if the reasoning still doesn't make sense

instancesByScaledCoordinate is a map, xMap is a map, but yMap is an array, am I reading it wrong?

I don't think 5c2c793 make it more readable unfortunately.
maybe just rename yMap to instancesArray? or simply instances?
Man this is so confusing...
I think the names should match the below for loops, but I'm not sure they do...

HarelM · 2025-11-10T21:12:51Z

Sorry for dragging this PR, I just really want to understand what you did here...

bradymadden97 · 2025-11-10T21:24:00Z

Sorry for dragging this PR, I just really want to understand what you did here...

Not a problem at all, I similarly want to make the code understandable so thank you for the feedback. Happy to keep iterating until it's in a good place.

melitele · 2026-01-10T02:07:08Z

        [crossTileID: number]: boolean;
    }) {
        const tolerance = this.tileID.canonical.z < newTileID.canonical.z ? 1 : Math.pow(2, this.tileID.canonical.z - newTileID.canonical.z);
+        const instancesByKey = this.groupSymbolInstancesByKey(symbolInstances);


If the map returned by groupSymbolInstancesByKey doesn't change for the same symbolInstances, the groupSymbolInstancesByKey can moved out of findMatches

melitele · 2026-01-10T19:27:16Z

The need for optimization stems from the fact that symbols don't have IDs unique across tiles. If they did, then cross tile symbol index would be a simple map. There is a mechanism that assigns IDs to features at least for GeoJSON source and symbols are unique per feature/layer combo.

HarelM · 2026-01-11T08:04:06Z

+        expect(childInstances.some(i => i.crossTileID === 0)).toBeFalsy();
+
+        // Overwrites the old id to match the already-added tile
+        index.addLayer(styleLayer, [mainTile, childTile], 0);


If this addLayer can happen earlier it would be great in order to keep the test in arrange-act-assert (AAA) style.
If this is not possible, consider splitting this test into two tests.

HarelM · 2026-01-11T08:04:25Z

+        }
+
+        // copies parent ids without duplicate ids in this tile
+        index.addLayer(styleLayer, [childTile], 0);


Same as previous comment.

melitele · 2026-01-12T02:15:08Z

The need for optimization stems from the fact that symbols don't have IDs unique across tiles. If they did, then cross tile symbol index would be a simple map. There is a mechanism that assigns IDs to features at least for GeoJSON source and symbols are unique per feature/layer combo.

Here's the commit that makes up cross tile ids for symbols from feature and layer ids. For features without ids it'll fall back to the code this PR is optimizing. For GeoJSON sources with features without id field, one can set either generateId or promoteId. Features in vector tiles generated from OSM tend to have ids. If not, cross tile ids can be generated for feature in vector tiles from geo coordinates and label (I haven't done this).

I modified the bin from this PR to use the cross tile ids from the commit above and label as feature id via generateId: https://jsbin.com/nemadaj/1/edit?html,output

In my ad hoc testing it doesn't stall and I am curious to see the level of actual improvement if @bradymadden97 can run the benchmark with the change from the commit above.

bradymadden97 · 2026-01-15T22:45:00Z

Tried to get the benchmarks above running again so I could test out the above suggestion, but running into errors even trying to build the benchmarks on main.

[!] (plugin typescript) RollupError: [plugin typescript] @rollup/plugin-typescript: Path of Typescript compiler option 'outDir' must be located inside Rollup 'dir' option.

Perhaps benchmarks has been broken?

HarelM · 2026-01-16T09:05:09Z

Are you using a different version of the libraries somehow? I think this error message if from a recent breaking change of the roll-up plugin.
Benchmarks are running on main every commit I believe:
https://github.com/maplibre/maplibre-gl-js/actions/workflows/upload-benchmarks.yml

HarelM · 2026-03-20T09:28:45Z

Any updates on this? What do we need in order to push this forward?

NeluQi · 2026-05-05T06:23:13Z

Any news on PR?

HarelM · 2026-05-06T05:31:37Z

Seems a bit abandoned... Do you want to copy the code over to a different PR and continue the work @NeluQi?

bradymadden97 · 2026-05-21T14:12:03Z

New implementation using @melitele's great suggestion here: #7665

bradymadden97 added 2 commits October 31, 2025 13:32

Optimize cross tile index searching

efaaea5

newline

c834bd2

bradymadden97 changed the title ~~Bmadden/cross tile symbol index optimization~~ Further optimize cross tile symbol index findMatches Oct 31, 2025

bradymadden97 marked this pull request as ready for review October 31, 2025 19:18

This was referenced Oct 31, 2025

Bad performance with many overlapping symbols #6192

Open

Zoom perfomance with symbol layer type #3084

Open

Optimize cross tile symbol index bradymadden97/maplibre-gl-js#1

Closed

HarelM reviewed Nov 1, 2025

View reviewed changes

Comment thread src/symbol/cross_tile_symbol_index.ts Outdated

HarelM reviewed Nov 1, 2025

View reviewed changes

Comment thread src/symbol/cross_tile_symbol_index.ts Outdated

bradymadden97 added 7 commits November 1, 2025 13:42

clarify comments

1c0d4f2

Fix iteration logic

4600214

Revert

6dda944

Merge branch 'bmadden/cross-tile-symbol-index-optimization' of https:…

77a9904

…//github.com/bradymadden97/maplibre-gl-js into bmadden/cross-tile-symbol-index-optimization

Add check to test

91ad4b2

Merge branch 'bmadden/add-check-to-cross-tile-symbol-index-test' into…

88ae4e0

… bmadden/cross-tile-symbol-index-optimization

Merge branch 'main' into bmadden/cross-tile-symbol-index-optimization

0c0f587

bradymadden97 requested a review from HarelM November 1, 2025 20:35

Merge branch 'main' into bmadden/cross-tile-symbol-index-optimization

392f7d3

HarelM reviewed Nov 3, 2025

View reviewed changes

Comment thread src/symbol/cross_tile_symbol_index.ts Outdated

HarelM reviewed Nov 3, 2025

View reviewed changes

Comment thread src/symbol/cross_tile_symbol_index.ts Outdated

bradymadden97 added 4 commits November 5, 2025 10:15

Merge branch 'main' into bmadden/cross-tile-symbol-index-optimization

c1dc44f

Merge branch 'bmadden/cross-tile-symbol-index-optimization' of https:…

99c7530

…//github.com/bradymadden97/maplibre-gl-js into bmadden/cross-tile-symbol-index-optimization

update for comments & add changelog

b0e7909

Merge branch 'main' into bmadden/cross-tile-symbol-index-optimization

1b92449

Update CHANGELOG.md

a954ea8

HarelM reviewed Nov 10, 2025

View reviewed changes

Comment thread src/symbol/cross_tile_symbol_index.test.ts Outdated

HarelM reviewed Nov 10, 2025

View reviewed changes

Comment thread src/symbol/cross_tile_symbol_index.ts Outdated

HarelM reviewed Nov 10, 2025

View reviewed changes

Comment thread src/symbol/cross_tile_symbol_index.ts

rename

5c2c793

bradymadden97 added 2 commits November 10, 2025 16:25

Merge branch 'main' into bmadden/cross-tile-symbol-index-optimization

880833a

pull out to method

f729a85

bradymadden97 commented Nov 10, 2025

View reviewed changes

Comment thread package-lock.json

bradymadden97 added 3 commits November 10, 2025 16:29

toBeDefined

3d218f3

Merge branch 'main' into bmadden/cross-tile-symbol-index-optimization

8ae7b9c

bump bundle size

7f28e73

melitele reviewed Jan 10, 2026

View reviewed changes

HarelM reviewed Jan 11, 2026

View reviewed changes

HarelM added the waiting for code review fixes label Mar 20, 2026

This was referenced May 20, 2026

Add CrossTileSymbolIndex benchmark #7664

Merged

Optimize CrossTileSymbolIndex by leveraging promoteId to automatically pair known feature symbols across tiles #7665

Draft

bradymadden97 closed this May 21, 2026

Uh oh!

Uh oh!

Conversation

bradymadden97 commented Oct 31, 2025 • edited by CommanderStorm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Demo of issue

Background

Changes in this PR

Before vs. After

Manual Regression Test

Benchmark

Related issues

Launch Checklist

Uh oh!

codecov Bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

HarelM commented Nov 1, 2025

Uh oh!

Uh oh!

Uh oh!

HarelM commented Nov 1, 2025

Uh oh!

bradymadden97 commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HarelM commented Nov 3, 2025

Uh oh!

Uh oh!

Uh oh!

HarelM commented Nov 3, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarelM Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HarelM commented Nov 10, 2025

Uh oh!

bradymadden97 commented Nov 10, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melitele commented Jan 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melitele commented Jan 12, 2026

Uh oh!

bradymadden97 commented Jan 15, 2026

Uh oh!

HarelM commented Jan 16, 2026

Uh oh!

HarelM commented Mar 20, 2026

Uh oh!

NeluQi commented May 5, 2026

Uh oh!

bradymadden97 commented Oct 31, 2025 •

edited by CommanderStorm

Loading

codecov Bot commented Oct 31, 2025 •

edited

Loading

bradymadden97 commented Nov 1, 2025 •

edited

Loading

HarelM Nov 10, 2025 •

edited

Loading