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

Track the validity of the frame and hit tester to avoid redundant work. #3043

Merged
merged 3 commits into from Sep 12, 2018

Conversation

@nical
Copy link
Collaborator

nical commented Sep 11, 2018

This lays the ground work for keeping track of the validity of the current frame and hit tester to allow only re-building them when something invalidates them.

This initial commit only skips redundant frame builds (typically when a scene is built and the vsync triggered transactions that follows doesn't scroll or use async animations). But after we separate building the frame and the hit tester, we'll be able to improve upon this by lazily building the frame and avoid double frame builds during scrolling and animations as well.


This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented Sep 11, 2018

try push: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=198630924&revision=f8c819bbfcb0f2576a028645fe911d18f348dcce

Also worth exploring as a followup: query the visibility of updated images so that we can skip frame building and rendering altogether if something animates off screen.

@nical nical changed the title Track the validity of teh frame and hit tester to avoid redundant work. Track the validity of the frame and hit tester to avoid redundant work. Sep 11, 2018
@nical
Copy link
Collaborator Author

nical commented Sep 11, 2018

/// Track whether the last built frame is up to date or if it will need to be re-built
/// before rendering again.
frame_is_valid: bool,
hit_tester_is_valid: bool,

This comment has been minimized.

@kvark

kvark Sep 11, 2018

Member

if it's not valid, can we just have hit_tester == None?

This comment has been minimized.

@nical

nical Sep 11, 2018

Author Collaborator

If we do this we need to first process all messages that use the hit tester before setting it to None. for example if you have a transaction that both updates and uses the hit tester, right now the rule is that hit testing operations use the previous hit tester and we update the hit tester at the end.
So it'll be a bit more fiddly but I think it's a good idea.

This comment has been minimized.

@nical

nical Sep 11, 2018

Author Collaborator

Alternatively, I'm leaning towards replacing the Option<HitTester> with a bare HitTester containing a valid boolean flag. This would let us recycle it's allocations instead of reallocating each time. Thoughts?

This comment has been minimized.

@kvark

kvark Sep 11, 2018

Member

everything for performance! :)

@kvark
kvark approved these changes Sep 11, 2018
@nical
Copy link
Collaborator Author

nical commented Sep 12, 2018

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Sep 12, 2018

📌 Commit 5e5a2be has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Sep 12, 2018

Testing commit 5e5a2be with merge 934ea1a...

bors-servo added a commit that referenced this pull request Sep 12, 2018
Track the validity of the frame and hit tester to avoid redundant work.

This lays the ground work for keeping track of the validity of the current frame and hit tester to allow only re-building them when something invalidates them.

This initial commit only skips redundant frame builds (typically when a scene is built and the vsync triggered transactions that follows doesn't scroll or use async animations). But after we separate building the frame and the hit tester, we'll be able to improve upon this by lazily building the frame and avoid double frame builds during scrolling and animations as well.

<!-- 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/3043)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 12, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 934ea1a to master...

@bors-servo bors-servo merged commit 5e5a2be into servo:master Sep 12, 2018
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Sep 13, 2018
Speed up updating the hit-test tree

The first two commits take advantage of what was put in place in #3043 to avoid building the frame when all we need is the hit tester.
The next two commits pick some low hanging optimization fruits in the hit tester to paper over a few minor talos regressions from the first commits.

<!-- 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/3052)
<!-- Reviewable:end -->
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

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