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

gfx: Implement display-list-based invalidation. #6213

Closed
wants to merge 2 commits into from

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented May 28, 2015

This ensures that we don't throw away all tiles if only a few of them need to be repainted. You can test this with something like servo -s 64 -Z paint-flashing tests/html/dlbi.html and mousing over the button.

Requires servo/rust-layers#167 to land first.

r? @glennw

Review on Reviewable

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 28, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5123

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@waddlesplash
Copy link

waddlesplash commented May 29, 2015

Just curious: do you have any numbers for how much faster this can be / is in practice?

@bors-servo
Copy link
Contributor

bors-servo commented May 29, 2015

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

@glennw
Copy link
Member

glennw commented Jun 1, 2015

Review status: all files reviewed, 4 unresolved discussions, some commit checks failed.
Reviewed files:

  • components/compositing/compositor.rs @ r2
  • components/compositing/compositor_layer.rs @ r1
  • components/gfx/display_list/invalidation.rs @ r1
  • components/gfx/display_list/mod.rs @ r2
  • components/gfx/paint_task.rs @ r2
  • components/layout/display_list_builder.rs @ r2
  • components/msg/compositor_msg.rs @ r1
  • components/util/range.rs @ r1
  • tests/html/dlbi.html @ r1

components/compositing/compositor.rs, line 658 [r2] (raw file):
Use unwrap or expect?


components/compositing/compositor.rs, line 680 [r2] (raw file):
unwrap or expect?


components/compositing/compositor.rs, line 1241 [r2] (raw file):
Perhaps the function should be "invalidate_and_send ... " to make it clear from reading calling code?


components/gfx/display_list/invalidation.rs, line 316 [r2] (raw file):
Is there a reason not to destructure the base item as you have for the items below?


Comments from the review on Reviewable.io

@glennw
Copy link
Member

glennw commented Jun 25, 2015

@pcwalton This seems to break the 3d transforms demos (stuff clips and doesn't update). I'll do some more investigation. tests/ref/iframe/positioning_margin is also failing with this PR (on my laptop at least).

@glennw
Copy link
Member

glennw commented Jun 29, 2015

@pcwalton I looked briefly into this - it appears to suffer the same problems as we currently have with clipping on 3d layers. How about updating this PR to disable DLBI on 3d layers (like we do for clipping currently)? That way, we can get this merged since it is useful for 2d layers anyway, and when I get to supporting clipping for 3d layers, I can also fix up the DLBI on 3d layers then?

@bors-servo
Copy link
Contributor

bors-servo commented Jul 1, 2015

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

@pcwalton pcwalton force-pushed the pcwalton:dlbi branch from 7b7bf18 to 2fa51a8 Jul 8, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 8, 2015

Rebased and attempted to turn this off for 3D layers.

@glennw How does this look?

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2015

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

@glennw
Copy link
Member

glennw commented Jul 12, 2015

@pcwalton Looks good to me, just needs a rebase.

@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 14, 2015

@bors-servo: r=glennw

@jdm jdm added S-tests-failed and removed S-awaiting-merge labels Jul 14, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 14, 2015

@bors-servo: retry

They may be intermittent. I'm going to retry to see if they are.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

Previous build results for android, gonk, linux2, mac1 are reusable. Rebuilding only linux1, linux3, mac2, mac3...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2015

💔 Test failed - mac2

@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 14, 2015

OK, so positioning_margin is not intermittent, but border_spacing is. That's what I had suspected/worried about. Is the border_spacing one pre-existing, I wonder?

@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 14, 2015

Actually, IIRC they're both intermittent; it's just that the iframe ones fail more often. I worry that DLBI is making our known compositor races worse :(

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2015

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

@jdm jdm added the S-needs-rebase label Jul 15, 2015
pcwalton added 2 commits May 28, 2015
Avoids repainting all tiles whenever any of them change.
@pcwalton pcwalton force-pushed the pcwalton:dlbi branch from d9d8900 to 7667944 Jul 31, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 31, 2015

@bors-servo: r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jul 31, 2015

📌 Commit 7667944 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jul 31, 2015

Testing commit 7667944 with merge a26a525...

bors-servo pushed a commit that referenced this pull request Jul 31, 2015
gfx: Implement display-list-based invalidation.

This ensures that we don't throw away all tiles if only a few of them need to be repainted. You can test this with something like `servo -s 64 -Z paint-flashing tests/html/dlbi.html` and mousing over the button.

Requires servo/rust-layers#167 to land first.

r? @glennw

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6213)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 31, 2015

💔 Test failed - linux1

@metajack
Copy link
Contributor

metajack commented Jul 31, 2015

/home/servo/buildbot/slave/linux1/build/tests/unit/util/lib.rs:5:35: 5:43 warning: unused or unknown feature, #[warn(unused_features)] on by default
/home/servo/buildbot/slave/linux1/build/tests/unit/util/lib.rs:5 #![feature(plugin, custom_derive, heap_api)]
                                                                                                   ^~~~~~~~
   Compiling servo v0.0.1 (file:///home/servo/buildbot/slave/linux1/build/components/servo)
../../tests/reftest.rs:46:26: 46:32 warning: use of deprecated item: superseded by split_first, #[warn(deprecated)] on by default
../../tests/reftest.rs:46     let mut parts = args.tail().split(|e| &**e == "--");
                                                   ^~~~~~
../../tests/reftest.rs:104:22: 104:42 warning: use of deprecated item: use process::exit instead, #[warn(deprecated)] on by default
../../tests/reftest.rs:104         Ok(false) => env::set_exit_status(1), // tests failed
                                                ^~~~~~~~~~~~~~~~~~~~
../../tests/reftest.rs:105:19: 105:39 warning: use of deprecated item: use process::exit instead, #[warn(deprecated)] on by default
../../tests/reftest.rs:105         Err(_) => env::set_exit_status(2),    // I/O-related failure
                                             ^~~~~~~~~~~~~~~~~~~~
@pcwalton
Copy link
Contributor Author

pcwalton commented Jul 31, 2015

That was the reftest I fixed… maybe it's a Linux-only failure now?

@bors-servo
Copy link
Contributor

bors-servo commented Jul 31, 2015

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

@nox nox added the S-needs-rebase label Sep 22, 2015
@glennw
Copy link
Member

glennw commented Nov 12, 2015

@pcwalton Should we close this and re-open it when you have time to look at this again?

@pcwalton
Copy link
Contributor Author

pcwalton commented Nov 12, 2015

Yeah, sure.

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

You can’t perform that action at this time.