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

[DO NOT MERGE] Box shadow performance optimization. #1891

Closed
wants to merge 2 commits into from

Conversation

@glennw
Copy link
Member

glennw commented Oct 18, 2017

Use a 9-patch for outset box shadows to improve performance.

This drastically reduces the size of the region that the blur
shader operates on. The blur shader itself is already much
slower than it should be, so this is a big performance win
on large box shadows.


This change is Reviewable

gw3583 added 2 commits Oct 17, 2017
Now that box shadows are rendered via a normal mask + blur,
we can just pass through radii for each corner to handle
this case. Right now, the brush mask assumes it will handle
the entire rect. The next step is to support segmenting
parts of the mask generation, which will greatly help with
the performance of the new box shadow implementation.

Fixes #1430.
This drastically reduces the size of the region that the blur
shader operates on. The blur shader itself is already much
slower than it should be, so this is a big performance win
on large box shadows.
@glennw
Copy link
Member Author

glennw commented Oct 18, 2017

The performance of box shadows can be quite bad with the new blur approach on very large shadows, due to a simplistic initial implementation aiming for correctness only.

My plan is to work on the proper segment / brush support discussed in previous issues over the next few days. Once that lands, the performance of the box shadows will be significantly faster than previously, and support all the new features of this implementation.

My preference would be to hold off for a few more days until the "proper" solution is in place to reduce the size of box shadows clips and blurs, but this is here in case we decide the current box shadow performance is intolerable, as a stopgap solution.

@glennw
Copy link
Member Author

glennw commented Oct 18, 2017

(This also includes #1889 which is still in the merge queue, so the patch isn't as big as it looks).

@glennw
Copy link
Member Author

glennw commented Oct 18, 2017

Note to self: doesn't scale with device pixel ratio correctly - need to update the cache image shader.

@kvark
Copy link
Member

kvark commented Oct 18, 2017

looks pretty good! ❤️


Reviewed 7 of 10 files at r2.
Review status: 7 of 18 files reviewed at latest revision, 2 unresolved discussions.


webrender/src/picture.rs, line 162 at r2 (raw file):

                let cache_height =
                    (prim_metadata.local_rect.size.height * prim_context.device_pixel_ratio).ceil() as i32;
                let cache_size = DeviceIntSize::new(cache_width, cache_height);

nit:
if we treat prim_context.device_pixel_ratio like ScaleFactor<f32, LayerSpace, DeviceSpace>, then this could be as simple as

let cache_size = (prim_metadata.local_rect.size * prim_context.device_pixel_ratio).ceil().to_i32();

webrender/src/tiling.rs, line 593 at r2 (raw file):

                            }
                            PictureKind::BoxShadow(..) => {
                                for i in 0..9 {

nit: would be nice to have some indication here on what 9 means. Perhaps, we could have an enum XXX: u32 that we can cast to u32 here?


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Oct 19, 2017

The latest upstream changes (presumably #1867) made this pull request unmergeable. Please resolve the merge conflicts.

@glennw
Copy link
Member Author

glennw commented Oct 19, 2017

Closing in favor of #1896 which is a simpler solution.

@glennw glennw closed this Oct 19, 2017
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

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