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

4x faster polygon compositing #91

Merged
merged 1 commit into from
Dec 3, 2018
Merged

4x faster polygon compositing #91

merged 1 commit into from
Dec 3, 2018

Conversation

springmeyer
Copy link
Contributor

This speeds up polygon decoding + clipping + encoding by 4x. Polygons are going to be the slowest to overzoom, so this is significant.

The speed boost comes from avoiding the allocation of intermediate vectors to store rings. Instead we collect bbox's while we decode, only clip geometries that are not fully within the target tile, and when we have results we encode them directly to the new feature.

Current master, for test 11 gives:

node bench/bench.js --iterations 500 --concurrency 1 --package vtcomposite
11: tiles completely made of polygons, overzooming and lots of properties ... 166 runs/s (3015ms)

This PR gives, for test 11, gives:

node bench/bench.js --iterations 500 --concurrency 1 --package vtcomposite
11: tiles completely made of polygons, overzooming and lots of properties ... 728 runs/s (687ms)

Going to ask @artemp for first PR review.

/cc @mapbox/maps-api @artemp @flippmoke @ericfischer

Copy link
Contributor

@artemp artemp left a comment

Choose a reason for hiding this comment

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

looks reasonable 👍

@artemp artemp merged commit ad3b875 into master Dec 3, 2018
@artemp artemp deleted the speed-up-poly-decode branch December 3, 2018 10:44
@springmeyer
Copy link
Contributor Author

@artemp any feedback or things you can see that could be improved?

@artemp
Copy link
Contributor

artemp commented Dec 3, 2018

@springmeyer - Only calling potentially 'expensive' intersection if interior ring is not entirely within bounding box sounds good to me as I mentioned. I also believe the same logic were used already in mapnik-vector-tile/node-mapnik - I'm sure I saw similar approach already. I'll comment more if I see any possibilities for improvements.

@e-n-f
Copy link
Contributor

e-n-f commented Dec 3, 2018

The one possible improvement I can suggest is that Tippecanoe's analogous check has three cases: entirely within the tile (no clipping needed), entirely outside the tile (immediately discarded), and overlapping the tile edge (needs to be clipped). If there are a lot of features that are entirely outside the clip region, it might help to add the quick-discard case here too.

@springmeyer
Copy link
Contributor Author

The one possible improvement I can suggest is that Tippecanoe's analogous check has three cases: entirely within the tile (no clipping needed), entirely outside the tile (immediately discarded), and overlapping the tile edge (needs to be clipped).

Thanks @ericfischer I don't think all these cases are tested and so I can't be sure at all whether the current behavior is correct (not buggy) or optimized for perf. So I think we should revert this work and keep in a PR until we can be sure.

@springmeyer springmeyer restored the speed-up-poly-decode branch December 8, 2018 21:04
springmeyer pushed a commit that referenced this pull request Dec 8, 2018
springmeyer added a commit that referenced this pull request Dec 8, 2018
revert #91 for now / until further tested
@springmeyer springmeyer deleted the speed-up-poly-decode branch December 8, 2018 21:17
@springmeyer
Copy link
Contributor Author

Reverted this landing in master prematurely in #91. Recreated #93 to track getting this landed.

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.

3 participants