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

Fix a picture caching race condition. #3424

Merged
merged 3 commits into from Dec 17, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Copy link
Collaborator

gw3583 commented Dec 17, 2018

Sometimes, the render backend produces frames faster than they
can be drawn by the renderer. The renderer has a code path for
this situation that drops frames in the render queue if there
are more recent frames to draw.

However, if the frame contains render tasks that will be persisted
in the texture cache, that frame must be executed in order to
ensure the texture cache is updated.

This patch extends that check to also consider render tasks that
will be blitted to the texture cache as a result of picture
caching.

This case occurs very rarely in real world usage, but does
occur quite commonly when running reftests on CI where the
time spent building simple frames is often much less than the
time for the software rasterizer to draw them.


This change is Reviewable

Fix a picture caching race condition.
Sometimes, the render backend produces frames faster than they
can be drawn by the renderer. The renderer has a code path for
this situation that drops frames in the render queue if there
are more recent frames to draw.

However, if the frame contains render tasks that will be persisted
in the texture cache, that frame must be executed in order to
ensure the texture cache is updated.

This patch extends that check to also consider render tasks that
will be blitted to the texture cache as a result of picture
caching.

This case occurs very rarely in real world usage, but _does_
occur quite commonly when running reftests on CI where the
time spent building simple frames is often much less than the
time for the software rasterizer to draw them.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 17, 2018

r? @kvark or anyone

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 17, 2018

Added a follow up commit that fixes an issue where opacity bindings on pictures were not being included in a tile's dependencies.

@kvark

kvark approved these changes Dec 17, 2018

Copy link
Member

kvark left a comment

Great find! 👏
I do wonder if with all those new must_be_drawn tasks we may end up with a situation where the renderer once again can't keep up with the frame builder?

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Dec 17, 2018

Leaving in case you want to launch a try ;) Feel free to proceed when ready please.

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 17, 2018

I don't think it'll be an issue in practice, but it's something to keep an eye out for certainly. Thanks for the quick review - try shouldn't be necessary since this still only affects the non-default render path for now.

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

📌 Commit 8ad3a04 has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

⌛️ Testing commit 8ad3a04 with merge 3a4ce4b...

bors-servo added a commit that referenced this pull request Dec 17, 2018

Auto merge of #3424 - gw3583:pic-race, r=kvark
Fix a picture caching race condition.

Sometimes, the render backend produces frames faster than they
can be drawn by the renderer. The renderer has a code path for
this situation that drops frames in the render queue if there
are more recent frames to draw.

However, if the frame contains render tasks that will be persisted
in the texture cache, that frame must be executed in order to
ensure the texture cache is updated.

This patch extends that check to also consider render tasks that
will be blitted to the texture cache as a result of picture
caching.

This case occurs very rarely in real world usage, but _does_
occur quite commonly when running reftests on CI where the
time spent building simple frames is often much less than the
time for the software rasterizer to draw them.

<!-- 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/3424)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

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

@bors-servo bors-servo merged commit 8ad3a04 into servo:master Dec 17, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@gw3583 gw3583 deleted the gw3583:pic-race branch Dec 17, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 17, 2018

Bug 1514744 - Update webrender to commit 3a4ce4b66a7bc9bd10773744d205…
…30c609a61e90 (WR PR #3424). r=kats

servo/webrender#3424

Differential Revision: https://phabricator.services.mozilla.com/D14743

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment