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

Stop unnecessary serialization of symbol source features. #7013

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Jul 23, 2018

Fixes issue #7012.
This data is expensive to serialize and isn't even being used.

I did a brief manual check to see that this didn't break anything. I'm still not sure why this was here in the first place, but I think it's (1) vestigial, and (2) got more expensive as part of adding expressions.

None of our benchmarks exercise the tile serialization pathway, but I did some quick-and-dirty benchmarking by zooming from 7 to 16 and back twice (40 seconds total animation) and counting frames. For each recording 600 == no dropped frames. The first run is presumably slower because the tiles are being loaded for the first time instead of from cache.

Before
562
594
600
598

After
563
594
589
597

🤷‍♂️ Not much of a difference? Let me try to focus on just first load since we'd expect the difference to mainly be there:

Before
562
558
562
534

-> 46 dropped frame avg

After
563
567
570
565

-> 34 dropped frame avg

If I squint enough that looks like a win!

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

/cc @mourner @anandthakker @ansis

Fixes issue #7012.
This data is expensive to serialize and isn't even being used.
@ChrisLoer ChrisLoer requested a review from mourner July 23, 2018 19:20
@mourner mourner merged commit f2d8685 into master Jul 23, 2018
@mourner mourner deleted the no-feature-serialize branch July 23, 2018 19:28
@ChrisLoer
Copy link
Contributor Author

I did another four runs of before/after and got generally slower results (not sure why: I did switch to a different internet connection and I suspect it being faster meant it loaded more tiles during the zoom animation?). But the difference between before and after got more pronounced. Across all 8 runs, "before" is now 67 dropped frames, stdev 29, "after" is 40 dropped frames, stdev 7.

Hooray for low-hanging fruit? 🎉

pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Sep 6, 2018
Fixes issue mapbox#7012.
This data is expensive to serialize and isn't even being used.
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Sep 11, 2018
Fixes issue mapbox#7012.
This data is expensive to serialize and isn't even being used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants