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

Reduce need for full world rect calculation for primitives. #3313

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Collaborator

gw3583 commented Nov 15, 2018

The fewer places that rely on a full world rect, the easier it
is to do clustered culling, where we accept / reject primitives
in groups.

The main use for world rects of primitivs is overlap calculations
during batching. Instead, switch to use the surface relative rect
for batching. This is safe since we know that when we merge batches
from different surfaces, they will never be overlapping in the
allocated render target.

Other changes:

  • Refactor the initial picture traversal to use a state object
    that maintains an internal stack of picture / surface info.
    This is easier to reason about, and will be helpful once we
    start using this to pass information about caching state.
  • Use world rect rather than clipped prim world rect for bounds
    during plane-splitting. This was landed previously but
    backed out due to an unrelated bug in that patch.
  • Change get_raster_rects to not calculate the transform, since
    most uses of this method don't require it.
  • Change conservative tiling calculations to use the world rect
    for bounds instead of clipped prim rect. All we're trying to
    do here is reject tiles outside the viewport, so this simplifies
    the code and removes the need for the primitive world rect in
    one more location.

This change is Reviewable

Reduce need for full world rect calculation for primitives.
The fewer places that rely on a full world rect, the easier it
is to do clustered culling, where we accept / reject primitives
in groups.

The main use for world rects of primitivs is overlap calculations
during batching. Instead, switch to use the surface relative rect
for batching. This is safe since we know that when we merge batches
from different surfaces, they will never be overlapping in the
allocated render target.

Other changes:
 * Refactor the initial picture traversal to use a state object
   that maintains an internal stack of picture / surface info.
   This is easier to reason about, and will be helpful once we
   start using this to pass information about caching state.
 * Use world rect rather than clipped prim world rect for bounds
   during plane-splitting. This was landed previously but
   backed out due to an unrelated bug in that patch.
 * Change get_raster_rects to not calculate the transform, since
   most uses of this method don't require it.
 * Change conservative tiling calculations to use the world rect
   for bounds instead of clipped prim rect. All we're trying to
   do here is reject tiles outside the viewport, so this simplifies
   the code and removes the need for the primitive world rect in
   one more location.
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 15, 2018

r? @kvark or @nical

This extracts some of the simpler parts from #3309 in order to hopefully get them landed first.

Pending try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=857098209d93fcfbe1af98e7686fc5525f9f98a3

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 15, 2018

Despite the amount of orange on the try run, I think this looks OK. There is one WR related crash in C but this appears to be the known get_relative_transform crash, and the others appear to be unrelated. Please double check though!

@kvark

kvark approved these changes Nov 15, 2018

Looks quite clean 👍 :lgtm:

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@kvark

This comment has been minimized.

Member

kvark commented Nov 15, 2018

In try push:

  • wpt6 isn't currently recognized as an intermittent, although it is likely
  • wr1's UNEXPECTED_FAIL also isn't marked as an intermittent, but this one doesn't look entirely unrelated

As for the crash, the "known" one, as far as I'm aware is the failed "invalid parent!" expectation. Your case is slightly different, not to mention that I didn't expect the old one to show up on CI (seemed deterministic, cc @aosmond ).

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 15, 2018

Thanks for the double check!

The wpt6 run definitely looks unrelated.

The Wr1 fail is one I see regularly, and it looks like there are intermittent bugs filed for it.

The crash that I'm talking about is https://bugzilla.mozilla.org/show_bug.cgi?id=1502717 - it's an intermittent array out of bounds error, which is the same as occurred here (and only occurred in one of the test configs).

@kvark

This comment has been minimized.

Member

kvark commented Nov 15, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

📌 Commit 3b2ed3d has been approved by kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

⌛️ Testing commit 3b2ed3d with merge 9ec08a4...

bors-servo added a commit that referenced this pull request Nov 15, 2018

Auto merge of #3313 - gw3583:prim-vis-bits, r=kvark
Reduce need for full world rect calculation for primitives.

The fewer places that rely on a full world rect, the easier it
is to do clustered culling, where we accept / reject primitives
in groups.

The main use for world rects of primitivs is overlap calculations
during batching. Instead, switch to use the surface relative rect
for batching. This is safe since we know that when we merge batches
from different surfaces, they will never be overlapping in the
allocated render target.

Other changes:
 * Refactor the initial picture traversal to use a state object
   that maintains an internal stack of picture / surface info.
   This is easier to reason about, and will be helpful once we
   start using this to pass information about caching state.
 * Use world rect rather than clipped prim world rect for bounds
   during plane-splitting. This was landed previously but
   backed out due to an unrelated bug in that patch.
 * Change get_raster_rects to not calculate the transform, since
   most uses of this method don't require it.
 * Change conservative tiling calculations to use the world rect
   for bounds instead of clipped prim rect. All we're trying to
   do here is reject tiles outside the viewport, so this simplifies
   the code and removes the need for the primitive world rect in
   one more location.

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

This comment has been minimized.

Contributor

bors-servo commented Nov 15, 2018

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

@bors-servo bors-servo merged commit 3b2ed3d into servo:master Nov 15, 2018

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 4 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment