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

Change how point_queue and active_edges are sorted for performance #335

Merged
merged 3 commits into from May 11, 2020

Conversation

@Veedrac
Copy link
Contributor

Veedrac commented May 10, 2020

My understanding is that every path, which is a set of connected sequences of Bezier curves, is individually tiled. The tiling procedure descends from appropriate nodes, splitting off line segments from its Beziers piece by piece, and splitting those line segments if they cross between a 16-tall strip. Once a strip has been allocated its line segments, it is partitioned into 16x16 tiles.

I believe point_queue is only sorted by y coordinate to coordinate work to one strip at a time. This is more efficiently handled by punting later work into a next_point_queue, and never explicitly sorting the data. This does mean you might punt future work several times before it is actually handled, but this is not an issue in examples I tested, and can be solved if need be.

Similarly I believe active_edges is only sorted by x coordinate for the tile-by-tile work in process_old_active_edges. It is much more efficient to sort once inside that function.

SortedVector is then only used in one place to sort gradients, so I removed it and inlined the functionality.

Speaking speculatively, it looks like it would be more efficient, both in CPU time and in the number of line segments generated, to partition the Bezier curves directly into the strips (you'd only need to solve a cubic), and then directly use a Bezier flattening algorithm on each segment. This has a few advantages: you partition exactly at tile boundaries, you don't need to redo partitioning work each time you bite a piece of the curve off, and you can use better partitioning algorithms (like this?).

One thing I don't understand is how the code handles Bezier curves that extend above the strip containing their endpoints. The code only seems to check the y coordinates of their endpoints.

@Veedrac Veedrac force-pushed the Veedrac:master branch 3 times, most recently from ad27978 to 96ac2c9 May 11, 2020
@Veedrac Veedrac changed the title Change how point_queue and active_edges are sorted for performance [WIP] Change how point_queue and active_edges are sorted for performance May 11, 2020
@pcwalton
Copy link
Collaborator

pcwalton commented May 11, 2020

One thing I don't understand is how the code handles Bezier curves that extend above the strip containing their endpoints. The code only seems to check the y coordinates of their endpoints.

These curves are eliminated prior to the tiling phase in make_monotonic.

Copy link
Collaborator

pcwalton left a comment

Wow, this is a ~30% win on paris-30k.svg! Very nice finds.

I think I'd prefer to keep around SortedVector as long as it has one use, just to encapsulate the sorting logic and to make it clear that the gradient stops are sorted. Other than removing SortedVector entirely, this looks great!

let index = self.stops.binary_search_by(|other| {
other.offset.partial_cmp(&stop.offset).unwrap()
}).unwrap_or_else(convert::identity);
self.stops.insert(index, stop);

This comment has been minimized.

@pcwalton

pcwalton May 11, 2020

Collaborator

Wouldn't it be faster to keep the old sorted vector code since insert has to be O(n) anyway?

This comment has been minimized.

@Veedrac

Veedrac May 11, 2020

Author Contributor

This is almost the same code SortedVector used, just inlined.

I did change unwrap_or(Less) to just unwrap(), and am sorting only on offset, which now that I think about it might not be valid. What's the expected behaviour for gradients with two points at the same offset?

This comment has been minimized.

@pcwalton

pcwalton May 11, 2020

Collaborator

Here's what the spec says:

If multiple stops are added at the same offset on a gradient, then they must be placed in the order added, with the first one closest to the start of the gradient, and each subsequent one infinitesimally further along towards the end point (in effect causing all but the first and last stop added at each point to be ignored).

This comment has been minimized.

@Veedrac

Veedrac May 11, 2020

Author Contributor

Ah, then both versions are actually wrong, since binary_search can return ‘any one of the matches’ to insert at. I'll add a fix.

renderer/src/tiles.rs Outdated Show resolved Hide resolved
Veedrac added 3 commits May 11, 2020
point_queue is only sorted by y coordinate to coordinate
work to one strip at a time. This is more efficiently handled
by punting later work into a next_point_queue,
and never explicitly sorting the data.

active_edges is only sorted by x coordinate for the tile-by-tile
work in process_old_active_edges. It is much more efficient
to sort once inside that function.
Zero-width gradient sections should be ‘ignored’.
The visible endpoints should be precisely the first
and last colors added at that point.

https://html.spec.whatwg.org/multipage/canvas.html#interpolation
@Veedrac Veedrac force-pushed the Veedrac:master branch from 2d49cbc to 033a150 May 11, 2020
@Veedrac
Copy link
Contributor Author

Veedrac commented May 11, 2020

Since ordering turns out to be rather subtle for gradients, it seems better to have the logic inline, rather than the responsibility of SortedVector.

@pcwalton pcwalton merged commit 33cd379 into servo:master May 11, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pcwalton
Copy link
Collaborator

pcwalton commented May 11, 2020

Agreed. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.