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

Script paint worklets speculative evaluation #17810

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Jul 20, 2017

This PR speculatively calls paint worklets during style, which increases the concurrency, since it increases the chance that the cache will have the right result in it when it comes to layout. The speculation is wasted effort if the size of the element has changed, but this is often not the case.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #17486 and #17369.
  • These changes do not require tests because it's a performance improvement

This change is Reviewable

@highfive
Copy link

highfive commented Jul 20, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/traversal.rs, components/style/context.rs
  • @canaltinova: components/style/traversal.rs, components/style/context.rs
  • @KiChjang: components/script/dom/paintrenderingcontext2d.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/bindings/trace.rs and 4 more
  • @fitzgen: components/script/dom/paintrenderingcontext2d.rs, components/script/dom/canvasrenderingcontext2d.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/bindings/trace.rs and 4 more
  • @emilio: components/layout/display_list_builder.rs, components/style/traversal.rs, components/style/context.rs, components/layout/context.rs
@highfive
Copy link

highfive commented Jul 20, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 20, 2017

This PR is dependent on #17634.

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-speculative-evaluation branch from 558002f to 5976728 Jul 20, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-speculative-evaluation branch from 5976728 to 372f159 Jul 21, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-speculative-evaluation branch from 372f159 to 5b61105 Jul 24, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 24, 2017

#17634 landed, so this PR is ready for review!

@bors-servo
Copy link
Contributor

bors-servo commented Jul 28, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-speculative-evaluation branch from 31a00a3 to 016af1a Jul 28, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jul 28, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-speculative-evaluation branch from 016af1a to ffd4c99 Jul 28, 2017
@emilio
emilio approved these changes Jul 28, 2017
Copy link
Member

emilio left a comment

Looks fine, just a question.

@@ -520,6 +522,8 @@ where
}
}

notify_paint_worklet(context, data);

This comment has been minimized.

Copy link
@emilio

emilio Jul 28, 2017

Member

Are you sure you want this outside of the if compute_self block? Do we really always need to paint it even if the style is the same?

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Jul 29, 2017

Author Member

Good point. Fixed.

@emilio
Copy link
Member

emilio commented Jul 28, 2017

Also, do you have any numbers on this?

@bors-servo
Copy link
Contributor

bors-servo commented Jul 29, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-speculative-evaluation branch from ffd4c99 to c512f6f Jul 29, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 29, 2017

@emilio I'm afraid not, to get decent numbers we'd need a page with reasonably complex style/layout and a paint worklet. All the paint worklet demos are quite simple at the moment :/

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

bors-servo commented Jul 29, 2017

📌 Commit c512f6f has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented Jul 29, 2017

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Jul 29, 2017

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

@bors-servo
Copy link
Contributor

bors-servo commented Jul 31, 2017

🔒 Merge conflict

@asajeffrey asajeffrey force-pushed the asajeffrey:script-paint-worklets-speculative-evaluation branch from c512f6f to 936dd3e Jul 31, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 31, 2017

No actual numbers, but at least the cache is doing its job!

$ RUST_LOG=script::dom::paintworkletglobalscope ./mach run -d --pref=dom.worklet.enabled http://googlechrome.github.io/houdini-samples/
DEBUG:script::dom::paintworkletglobalscope: Creating paint worklet global scope for pipeline (0,2).
DEBUG:script::dom::paintworkletglobalscope: Registering paint image name ripple.
DEBUG:script::dom::paintworkletglobalscope: Registering definition ripple.
DEBUG:script::dom::paintworkletglobalscope: Creating paint worklet global scope for pipeline (0,2).
DEBUG:script::dom::paintworkletglobalscope: Registering paint image name ripple.
DEBUG:script::dom::paintworkletglobalscope: Registering definition ripple.
DEBUG:script::dom::paintworkletglobalscope: Creating paint worklet global scope for pipeline (0,2).
DEBUG:script::dom::paintworkletglobalscope: Registering paint image name ripple.
DEBUG:script::dom::paintworkletglobalscope: Registering definition ripple.
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(0,0) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Cache miss on paint worklet ripple!
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(300,300) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(300,300) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Cache hit on paint worklet ripple!
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(300,300) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Cache hit on paint worklet ripple!
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(300,300) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Cache hit on paint worklet ripple!
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(300,300) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Cache hit on paint worklet ripple!
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(300,300) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Cache hit on paint worklet ripple!
DEBUG:script::dom::paintworkletglobalscope: Cache hit on paint worklet ripple!
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(300,300) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Cache hit on paint worklet ripple!
DEBUG:script::dom::paintworkletglobalscope: Invoking a paint callback ripple(300,300) at 1.
DEBUG:script::dom::paintworkletglobalscope: Invoking paint function ripple.
DEBUG:script::dom::paintworkletglobalscope: Cache hit on paint worklet ripple!

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

bors-servo commented Jul 31, 2017

📌 Commit 936dd3e has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented Jul 31, 2017

Testing commit 936dd3e with merge cf5602e...

bors-servo added a commit that referenced this pull request Jul 31, 2017
…valuation, r=emilio

Script paint worklets speculative evaluation

<!-- Please describe your changes on the following line: -->

This PR speculatively calls paint worklets during style, which increases the concurrency, since it increases the chance that the cache will have the right result in it when it comes to layout. The speculation is wasted effort if the size of the element has changed, but this is often not the case.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #17486 and #17369.
- [X] These changes do not require tests because it's a performance improvement

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17810)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 31, 2017

@bors-servo bors-servo merged commit 936dd3e into servo:master Jul 31, 2017
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Jul 31, 2017
4 of 4 tasks complete
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.

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