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

Optimize and simplify symbol placement code #12351

Merged
merged 13 commits into from
Nov 14, 2022
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Nov 1, 2022

While I'm out of power and Internet, I'm going to be working offline on optimizing and simplifying symbol placement code, which is terrifyingly complex at the moment and in dire need of cleanup. Will try to do that in self-contained commits and push periodically when I'm back online.

Results

Zooming around z14–16 on streets-v11 SF with lots of roads in view, looking at profiler results, this PR seems to improve performance of placement along lines by ~40–50%.

Before:
image

After:
image

Benchmarks don't show a convincing win, just tiny improvements close to statistical noise — I guess they don't hit label-dense areas enough, but it's still an improvement (most diagnostic metrics show a positive change, even if tiny):

The PR also cuts down the bundle size a little (-314 bytes) and improves flow types in a few places around symbol code (replacing any with proper typing).

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 inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Improve performance of text and icon placement.</changelog>

@mourner mourner added the performance ⚡ Speed, stability, CPU usage, memory usage, or power usage label Nov 1, 2022
@mourner mourner force-pushed the optimize-symbol-placement branch from e447445 to fb33b04 Compare November 1, 2022 19:54
@mourner mourner force-pushed the optimize-symbol-placement branch from 11c161d to 59e7d1f Compare November 7, 2022 15:55
@mourner mourner marked this pull request as ready for review November 8, 2022 10:53
@mourner mourner requested a review from a team as a code owner November 8, 2022 10:53
@mourner
Copy link
Member Author

mourner commented Nov 8, 2022

Hold on while I'm fixing up the last commit...

@mourner mourner force-pushed the optimize-symbol-placement branch from 5e6b175 to a5d2ee6 Compare November 9, 2022 09:06
@mourner mourner force-pushed the optimize-symbol-placement branch from e5ffa31 to 6cec89d Compare November 9, 2022 13:38
@mourner mourner requested review from karimnaaji and ansis November 9, 2022 17:48
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

A first pass! I still have to carefully go through more of the changes. But this looks really good to me already, a lot of nice simplifications.

src/data/bucket/symbol_attributes.js Show resolved Hide resolved
src/symbol/projection.js Outdated Show resolved Hide resolved
src/symbol/projection.js Show resolved Hide resolved
@mourner mourner force-pushed the optimize-symbol-placement branch from 675a8a1 to 5482556 Compare November 10, 2022 11:20
@mourner mourner changed the title Optimize symbol placement code Optimize and simplify symbol placement code Nov 10, 2022
@mourner mourner force-pushed the optimize-symbol-placement branch from 049b7eb to 536db49 Compare November 10, 2022 12:26
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

I finished going over the change and left a few more minor comments/questions. But this was a really needed cleanup and glad to see that we can get out some noticeable performance gain at the same time, LGTM!

src/symbol/projection.js Show resolved Hide resolved
src/symbol/projection.js Show resolved Hide resolved
@mourner mourner merged commit 2065909 into main Nov 14, 2022
@mourner mourner deleted the optimize-symbol-placement branch November 14, 2022 16:17
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.

3 participants