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 line tessellation code #8303

Merged
merged 9 commits into from
Aug 20, 2019
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented May 30, 2019

This PR refactors the line tessellation code, removing code repetition and some unnecessary logic, making it both faster and easier to follow.

The potential performance improvement comes from reducing the amount of temporary Point object allocations by ~2 times, which theoretically reduces the amount of subsequent GC pauses. I'll try to find a way to measure real-world performance impact, but roughly it should be 5-10% less time spent tessellating lines on the worker side.

It brings some ideas from #7085 but in a less intrusive way.

The PR is made on top of #8275 so needs to be applied after it lands.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality already covered
  • document any changes to public APIs no changes
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/gl-native — might need a port to native to make the code between platforms more similar.

@mourner mourner marked this pull request as ready for review June 5, 2019 19:03
@mourner mourner requested a review from ansis June 5, 2019 19:16
@mourner mourner force-pushed the improve-round-join-tessellation branch from 0aeafdc to 858e45a Compare July 23, 2019 13:04
@mourner mourner changed the base branch from improve-round-join-tessellation to master July 30, 2019 17:22
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

This looks great! just that one small question.

It's amazing how much unnecessary stuff was in there. This should be much easier to understand.

this.distance = 0;
this.scaledDistance = 0;
this.totalDistance = 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think creating a line for each object is a significant cost? putting per-line values directly on the bucket seems a bit less nice but probably ok if there is a significant benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ansis we had distance as a bucket state before this PR, so the latter doesn't change the status quo in that sense — I moved the line stats to shared state too for simplicity/consistency, but I think we can revisit this in a follow-up PR.

@mourner
Copy link
Member Author

mourner commented Aug 20, 2019

Layout benchmarks are not affected:

image

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