Skip to content

Fix performance issue when placing many symbols on terrain #11466

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

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

mourner
Copy link
Member

@mourner mourner commented Feb 3, 2022

Exploring profiling results with @stepankuzmin, we found that an abnormal amount of CPU time was spent fetching terrain elevation values when placing symbols on terrain, and there was a lot of GC related to it as well. This PR attempts to fix the issue by avoiding creating a typed array view within DemData get, a method on a very hot path.

This also required some minor refactoring so that the class with the corresponding changes can be properly transferred from the worker to the main thread (now we hold on to a Uint8Array view rather than Uint32Array, because we can't transfer with both references).

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • post benchmark scores
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry for inclusion in the mapbox-gl-js changelog: (in title)

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
renovate-bot Mend Renovate
@mourner mourner requested a review from stepankuzmin February 3, 2022 14:53
@mourner mourner added the performance ⚡ Speed, stability, CPU usage, memory usage, or power usage label Feb 3, 2022
@mourner mourner marked this pull request as ready for review February 3, 2022 16:52
@mourner
Copy link
Member Author

mourner commented Feb 3, 2022

The rendering-3d benchmark results are indeterminate, but perhaps they don't cover the fringe cases that we've seen that are affected by this PR. https://sites.mapbox.com/benchmap-js-staging2-site/runs/448

@mourner mourner merged commit bfeb8f9 into main Feb 3, 2022
@mourner mourner deleted the fix-terrain-placement-perf branch February 3, 2022 17:35
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.

None yet

2 participants