Skip to content
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

Improve performance of symbol layers with identical or no text #12669

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

mourner
Copy link
Member

@mourner mourner commented Apr 19, 2023

Closes #7973. There was an accidental quadratic behavior in CrossTileSymbolIndex (responsible for deduplicating symbols across tiles) when a map had a ton of symbols with the same text (or no text, just an icon).

This PR changes the logic so that instead of indexing symbol instances by key (which is a numeric hash of its text), we make a KDBush index and search for duplicate symbols spatially. KDBush is already included as a dependency of Supercluster, so it doesn't increase the bundle size, and it's pretty fast, so the approach should work well for both the "lots of identical icons" (added in the debug/symbols-generated.html debug page) and "lots of different text labels" (e.g. Streets) cases.

In theory, this should also improve memory footprint for Streets because we avoid creating thousands of small arrays and a big object with key to array mapping — KDBush is much more memory-efficient. We should run a benchmark before merging to make sure it doesn't accidentally regress anything.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port — @alexshalamov this affects Native too because it has the same logic.
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Improve performance of symbol layers with identical or no text</changelog>

@mourner mourner added the performance ⚡ Speed, stability, CPU usage, memory usage, or power usage label Apr 19, 2023
@mourner mourner requested a review from a team as a code owner April 19, 2023 16:42
Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! cc/ @endanke

@endanke
Copy link
Contributor

endanke commented Apr 20, 2023

@alexshalamov I'm already on it :) (picked up the parity ticket)

@mourner
Copy link
Member Author

mourner commented Apr 20, 2023

Checked the benchmark runs and they're looking OK, so I'm merging. There's an uptick in array buffer count, but that should be OK because at the same time the total amount of objects is reduced. I'll also consider a further improvement in memory footprint by landing mourner/kdbush#32 and then reorganizing the logic here to hold key/crossTileID data in flat numeric arrays instead of an array of objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow performance with lots of empty labels
4 participants