Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upEmbed the PicturePrimitive inside BrushPrimitive. #2961
Conversation
|
Never fear, this commit is actually quite small! It just looks big as it includes the commit from #2951. Pending try: r? @kvark or anyone (just the last commit - the first commit is being reviewed separately). |
| frame_output_pipeline_id, | ||
| apply_local_clip_rect, | ||
| ); | ||
| pub fn get_pic(&self, index: PrimitiveIndex) -> &PicturePrimitive { |
This comment has been minimized.
This comment has been minimized.
kvark
Aug 9, 2018
Member
This looks like a regression in our internal APIs. Where we had separate type for picture index, we were able to associate it with a picture, now we pass more PrimitiveIndex around. I don't see a way around it if you want to move the picture under primitive metadata, but just pointing out at the issue.
This comment has been minimized.
This comment has been minimized.
gw3583
Aug 9, 2018
Author
Collaborator
I agree - this is an internal regression. However, I think it should be temporary only - I'd like to embed more of the prim store types directly into the runs struct in the Picture (see rationale above) which should then remove (most of?) the indexing that occurs here, if all goes according to plan.
| let brush = &self.cpu_brushes[metadata.cpu_prim_index.0]; | ||
| match brush.kind { | ||
| BrushKind::Picture(ref pic) => pic, | ||
| _ => unreachable!("bug: not a picture!"), |
This comment has been minimized.
This comment has been minimized.
kvark
Aug 9, 2018
Member
nit: I think if there is any doubt that this is reachable, then panic! is more appropriate :)
This comment has been minimized.
This comment has been minimized.
|
Fixed nit and rebased on top of the review comments for #2951. I'll rebase again once that is merged. |
Remove the cpu_pictures array and store the PicturePrimitive directly inside the BrushPrimitive. The rationale here is to move the prim store arrays to be stored and owned by each PicturePrimitive. This is an interim step towards that - it's a bit messy in places due to borrow check issues, but should be a lot tidier once complete. The benefit of doing this is that as we start to cache Pictures more aggressively, we can retain the PicturePrimitive struct from a previous frame, and easily compare the embedded primitive runs to see if we have a cache hit. This will make it possible to cache Picture primitives between *display lists* rather than just scroll frames where appropriate, by doing a deep compare on the primitive runs in a picture.
|
I did a fresh try run after rebasing, that looks green apart from unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d62530193f44ce1f131d85ce5791f66d6f9abec @bors-servo r=kvark |
|
|
Embed the PicturePrimitive inside BrushPrimitive. <!-- Reviewable:start --> This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2961) <!-- Reviewable:end -->
|
|
gw3583 commentedAug 9, 2018
•
edited by larsbergstrom
This change is