Don't build layers if the display list hasn't changed

RESOLVED FIXED in Firefox 60

Status

()

P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed)

Details

Attachments

(7 attachments)

59 bytes, text/x-review-board-request
tnikkel
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
miko
: review+
Details
59 bytes, text/x-review-board-request
miko
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
miko
: review+
Details
(Assignee)

Description

a year ago
When an invalidation happens, we schedule a paint, build the display list and then use Layer building/DLBI to find out if the invalidation actually affects anything visible.

With retained-dl, we have the ability to know if anything changed much earlier in the process, so we should take advantage of this and skip Layer building entirely for the no-op case.

This should reduce the CPU usage for offscreen animations (especially JS ones that aren't helped by throttling).
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
Comment on attachment 8941995 [details]
Bug 1429932 - Part 1: Remove mFireAfterPaintEvents and use mTransactions instead.

https://reviewboard.mozilla.org/r/212194/#review217988


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/base/nsPresContext.cpp:2615
(Diff revision 1)
>    nsPresContext* pc;
>    for (pc = this; pc; pc = pc->GetParentPresContext()) {
> -    if (pc->mFireAfterPaintEvents)
> +    TransactionInvalidations* transaction = pc->GetInvalidations(aTransactionId);
> +    if (transaction) {
>        break;
> -    pc->mFireAfterPaintEvents = true;
> +    } else {

Warning: Do not use 'else' after 'break' [clang-tidy: readability-else-after-return]

Comment 9

a year ago
mozreview-review
Comment on attachment 8941997 [details]
Bug 1429932 - Part 3: Refactor RetainedDisplayListBuilder::AttemptPartialUpdate to have an early return instead of a nested scope.

https://reviewboard.mozilla.org/r/212198/#review217990


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/painting/RetainedDisplayListBuilder.cpp:827
(Diff revision 1)
>  }
>  
> +class AutoClearFramePropsArray
> +{
> +public:
> +  AutoClearFramePropsArray()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: layout/painting/RetainedDisplayListBuilder.cpp:834
(Diff revision 1)
> +
> +  explicit AutoClearFramePropsArray(nsTArray<nsIFrame*>&& aArray)
> +    : mFrames(Move(aArray))
> +  {}
> +
> +  ~AutoClearFramePropsArray()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Comment 10

a year ago
mozreview-review
Comment on attachment 8941995 [details]
Bug 1429932 - Part 1: Remove mFireAfterPaintEvents and use mTransactions instead.

https://reviewboard.mozilla.org/r/212194/#review218106
Attachment #8941995 - Flags: review?(tnikkel) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8941997 [details]
Bug 1429932 - Part 3: Refactor RetainedDisplayListBuilder::AttemptPartialUpdate to have an early return instead of a nested scope.

https://reviewboard.mozilla.org/r/212198/#review218146

::: layout/painting/RetainedDisplayListBuilder.cpp:839
(Diff revision 1)
> +  ~AutoClearFramePropsArray()
> +  {
> +    ClearFrameProps(mFrames);
> +  }
> +
> +  operator nsTArray<nsIFrame*>&() { return mFrames; }

Could we instead of a cast operator, have this as function Frames() instead?
Then we could replace all the references to modifiedFrames.mFrames with modifiedFrames.Frames(), and make mFrames member variable private.

Alternatively, we could keep the raw arrays as they are, and have AutoClearFramePropsArray() keep references to them.

::: layout/painting/RetainedDisplayListBuilder.cpp:859
(Diff revision 1)
>    mBuilder.EnterPresShell(mBuilder.RootReferenceFrame());
>  
> -  nsTArray<nsIFrame*> modifiedFrames =
> -    GetModifiedFrames(&mBuilder);
> +  // We set the override dirty regions during ComputeRebuildRegion or in
> +  // nsLayoutUtils::InvalidateForDisplayPortChange. The display port change also
> +  // marks the frame modified, so those regions are cleared here as well.
> +  AutoClearFramePropsArray modifiedFrames(Move(GetModifiedFrames(&mBuilder)));

nit: I think Move() is not needed here since GetModifiedFrames() returns an rvalue.
Attachment #8941997 - Flags: review?(mikokm) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8941998 [details]
Bug 1429932 - Part 4: Determine when AttemptPartialUpdate made no changes, and return the result to the caller.

https://reviewboard.mozilla.org/r/212200/#review218162

::: layout/painting/RetainedDisplayListBuilder.cpp:393
(Diff revision 1)
>    // We have similar data in the nsIFrame DisplayItems() property, but it doesn't
>    // know which display list items are in, and we only want to match items in
>    // this list.
>    nsDataHashtable<DisplayItemHashEntry, nsDisplayItem*> oldListLookup(aOldList->Count());
>  
> +  if (!aNewList->IsEmpty()) {

Nice catch!

::: layout/painting/RetainedDisplayListBuilder.cpp:397
(Diff revision 1)
>  
> +  if (!aNewList->IsEmpty()) {
> -  for (nsDisplayItem* i = aOldList->GetBottom(); i != nullptr; i = i->GetAbove()) {
> +    for (nsDisplayItem* i = aOldList->GetBottom(); i != nullptr; i = i->GetAbove()) {
> -    i->SetReused(false);
> +      i->SetReused(false);
>  
> -    if (!aNewList->IsEmpty()) {
> +      if (!aNewList->IsEmpty()) {

This condition can be removed since it's the same as the surrounding one.

::: layout/painting/RetainedDisplayListBuilder.cpp:863
(Diff revision 1)
>    bool IsEmpty() const { return mFrames.IsEmpty(); }
>  
>    nsTArray<nsIFrame*> mFrames;
>  };
>  
> -bool
> +auto

Is there a reason to use this form instead of RetainedDisplayListBuilder::PartialUpdateResult?

::: layout/painting/RetainedDisplayListBuilder.cpp:910
(Diff revision 1)
> -    return false;
> +    return PartialUpdateResult::Failed;
>    }
>  
>    modifiedDirty.IntersectRect(modifiedDirty, mBuilder.RootReferenceFrame()->GetVisualOverflowRectRelativeToSelf());
>  
> -  PreProcessDisplayList(&mList, modifiedAGR);
> +  PartialUpdateResult result = PartialUpdateResult::Updated;

nit: I think it would be nicer to default to PartialUpdateResult::NoChange and invert the condition below.
Then PreProcessDisplayList() or MergeDisplayLists() could flip the flag to PartialUpdateResult::Updated if needed.
Attachment #8941998 - Flags: review?(mikokm) → review+

Comment 13

a year ago
mozreview-review
Comment on attachment 8942001 [details]
Bug 1429932 - Part 7: Restrict dirty regions in ComputeRebuildRegion to the overflow area of the current frame so that we discard invalidations that aren't visible.

https://reviewboard.mozilla.org/r/212206/#review218164

::: layout/painting/RetainedDisplayListBuilder.cpp:642
(Diff revision 1)
>  RetainedDisplayListBuilder::ComputeRebuildRegion(nsTArray<nsIFrame*>& aModifiedFrames,
>                                                   nsRect* aOutDirty,
>                                                   AnimatedGeometryRoot** aOutModifiedAGR,
>                                                   nsTArray<nsIFrame*>* aOutFramesWithProps)
>  {
> -  CRR_LOG("Computing rebuild regions for %d frames:\n", aModifiedFrames.size());
> +  CRR_LOG("Computing rebuild regions for %d frames:\n", aModifiedFrames.Length());

Could you also replace %d with the correct format %zu?

::: layout/painting/RetainedDisplayListBuilder.cpp:695
(Diff revision 1)
>        overflow = nsLayoutUtils::TransformFrameRectToAncestor(currentFrame, overflow, mBuilder.RootReferenceFrame(),
>                                                               nullptr, nullptr,
>                                                               /* aStopAtStackingContextAndDisplayPort = */ true,
>                                                               &currentFrame);
>        MOZ_ASSERT(currentFrame);
> +      overflow.IntersectRect(overflow, currentFrame->GetVisualOverflowRectRelativeToSelf());

I think that after this change, we do not mark stacking contexts with modified placeholders visible.

Isn't this a problem until bug 1428993 is fixed?

Comment 14

a year ago
mozreview-review
Comment on attachment 8942001 [details]
Bug 1429932 - Part 7: Restrict dirty regions in ComputeRebuildRegion to the overflow area of the current frame so that we discard invalidations that aren't visible.

https://reviewboard.mozilla.org/r/212206/#review218932
Attachment #8942001 - Flags: review?(mikokm) → review+

Comment 15

a year ago
mozreview-review
Comment on attachment 8941996 [details]
Bug 1429932 - Part 2: nsDisplayMask can paint images and shouldn't have the TYPE_RENDERS_NO_IMAGES flag.

https://reviewboard.mozilla.org/r/212196/#review221456
Attachment #8941996 - Flags: review?(mstange) → review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8941999 [details]
Bug 1429932 - Part 5: Move the Layer building section of nsDisplayList::PaintRoot into a separate function.

https://reviewboard.mozilla.org/r/212202/#review221460
Attachment #8941999 - Flags: review?(mstange) → review+

Comment 17

a year ago
mozreview-review
Comment on attachment 8942000 [details]
Bug 1429932 - Part 6: Attempt to skip Layer building if the display list hasn't changed.

https://reviewboard.mozilla.org/r/212204/#review221462
Attachment #8942000 - Flags: review?(mstange) → review+

Comment 18

a year ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65b8996eb50c
Part 1: Remove mFireAfterPaintEvents and use mTransactions instead. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb1394cba514
Part 2: nsDisplayMask can paint images and shouldn't have the TYPE_RENDERS_NO_IMAGES flag. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/09e5e25d48ee
Part 3: Refactor RetainedDisplayListBuilder::AttemptPartialUpdate to have an early return instead of a nested scope. r=miko
https://hg.mozilla.org/integration/mozilla-inbound/rev/72852fdf476c
Part 4: Determine when AttemptPartialUpdate made no changes, and return the result to the caller. r=miko
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f801e78f798
Part 5: Move the Layer building section of nsDisplayList::PaintRoot into a separate function. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/469ceaefd7a4
Part 6: Attempt to skip Layer building if the display list hasn't changed. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ac71407500
Part 7: Restrict dirty regions in ComputeRebuildRegion to the overflow area of the current frame so that we discard invalidations that aren't visible. r=miko
I see reftest failures locally on clipPath-and-mask-on-outflowElement-01b.html without any changes.
The failures I was seeing were caused by disabling e10s. I can reproduce properly if I don't disable-e10s.

That being said, when painting from DrawWindow
I get a display list of:
SolidColor p=0x7fe5bc8f0960 f=0x7fe5bc57f020(Viewport(-1)) key=48 bounds(0,0,48000,60000) layerBounds(0,0,48000,60000) visible(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform ref=0x7fe5bc57f020 agr=0x7fe5bc57f020 (opaque 0,0,48000,60000) (rgba 255,255,255,255)
LayerEventRegions p=0x7fe5bc8f0020 f=0x7fe5bc57f020(Viewport(-1)) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x7fe5bc57f020 agr=0x7fe5bc57f020 (hitRegion < (x=0, y=0, w=48000, h=60000); >)
LayerEventRegions p=0x7fe5bc8f01e0 f=0x7fe5bc57f0b0(Canvas(html)(-1)) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f01a0 <0,0,48000,60000> [0x7fe5bc57f1e0], 0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0 (hitRegion < (x=0, y=0, w=48000, h=60000); >)
CanvasBackgroundColor p=0x7fe5bc8f02e0 f=0x7fe5bc57f0b0(Canvas(html)(-1)) key=15 bounds(0,0,48000,60000) layerBounds(0,0,48000,60000) visible(0,0,48000,60000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f01a0 <0,0,48000,60000> [0x7fe5bc57f1e0], 0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) uniform ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0 (opaque 0,0,48000,60000) (rgba 255,255,255,255)

vs the following when painting before:

SolidColor p=0x7fe5bc8f0960 f=0x7fe5bc57f020(Viewport(-1)) key=48 bounds(0,0,48000,60000) layerBounds(0,0,48000,60000) visible(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform ref=0x7fe5bc57f020 agr=0x7fe5bc57f020 (opaque 0,0,48000,60000) (rgba 255,255,255,255)
LayerEventRegions p=0x7fe5bc8f0020 f=0x7fe5bc57f020(Viewport(-1)) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x7fe5bc57f020 agr=0x7fe5bc57f020 (hitRegion < (x=0, y=0, w=48000, h=60000); >)
LayerEventRegions p=0x7fe5bc8f01e0 f=0x7fe5bc57f0b0(Canvas(html)(-1)) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f01a0 <0,0,48000,60000> [0x7fe5bc57f1e0], 0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0 (hitRegion < (x=0, y=0, w=48000, h=60000); >)
CanvasBackgroundColor p=0x7fe5bc8f02e0 f=0x7fe5bc57f0b0(Canvas(html)(-1)) key=15 bounds(0,0,48000,60000) layerBounds(0,0,48000,60000) visible(0,0,48000,60000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f01a0 <0,0,48000,60000> [0x7fe5bc57f1e0], 0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) uniform ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0 (opaque 0,0,48000,60000) (rgba 255,255,255,255)
WrapList p=0x7fe5bc8f0860 f=0x7fe5bc57fd90(Block(div)(1) id:clipped) key=68 bounds(0,0,24000,24000) layerBounds(0,0,24000,24000) visible(0,0,24000,24000) componentAlpha(0,0,0,0) clip() asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0
  Mask p=0x7fe5bc8f0420 f=0x7fe5bc57fd90(Block(div)(1) id:clipped) key=51 bounds(0,0,24000,24000) layerBounds(0,0,24000,24000) visible(0,0,24000,24000) componentAlpha(0,0,0,0) clip() asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0 effects=(clip(non-trivial))
    WrapList p=0x7fe5bc8f0760 f=0x7fe5bc57fe40(Block(div)(1) id:absolutePosition) key=68 bounds(0,0,24000,24000) layerBounds(0,0,24000,24000) visible(0,0,24000,24000) componentAlpha(0,0,0,0) clip() asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0
      LayerEventRegions p=0x7fe5bc8f0520 f=0x7fe5bc57fe40(Block(div)(1) id:absolutePosition) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,24000,24000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f01a0 <0,0,48000,60000> [0x7fe5bc57f1e0], 0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0 (hitRegion < (x=0, y=0, w=24000, h=24000); >)
      BackgroundColor p=0x7fe5bc8f0660 f=0x7fe5bc57fe40(Block(div)(1) id:absolutePosition) key=4 bounds(0,0,24000,24000) layerBounds(0,0,24000,24000) visible(0,0,24000,24000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr(<0x7fe5bc57f1e0>) clipChain(0x7fe5bc8f01a0 <0,0,48000,60000> [0x7fe5bc57f1e0], 0x7fe5bc8f0160 <0,0,48000,60000> [root asr]) uniform ref=0x7fe5bc57f020 agr=0x7fe5bc57f0b0 (opaque 0,0,24000,24000) (rgba 1,1,0,1)

Comment 22

a year ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ffcc432d0c
Part 1: Remove mFireAfterPaintEvents and use mTransactions instead. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/db244e692584
Part 2: nsDisplayMask can paint images and shouldn't have the TYPE_RENDERS_NO_IMAGES flag. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d146aec735e
Part 3: Refactor RetainedDisplayListBuilder::AttemptPartialUpdate to have an early return instead of a nested scope. r=miko
https://hg.mozilla.org/integration/mozilla-inbound/rev/c91a2b710a65
Part 4: Determine when AttemptPartialUpdate made no changes, and return the result to the caller. r=miko
https://hg.mozilla.org/integration/mozilla-inbound/rev/c48601351975
Part 5: Move the Layer building section of nsDisplayList::PaintRoot into a separate function. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7a4df5efcc
Part 6: Attempt to skip Layer building if the display list hasn't changed. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/24cf388d5d5b
Part 7: Restrict dirty regions in ComputeRebuildRegion to the overflow area of the current frame so that we discard invalidations that aren't visible. r=miko
(Assignee)

Updated

a year ago
Depends on: 1437374
Depends on: 1437436

Comment 24

a year ago
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/cb9b9f2707a3
Backed out 7 changesets for frequently failing layout/reftests/table-background/backgr_layers-opacity.html a=backout
Backed out 7 changesets (bug 1429932) for frequently failing layout/reftests/table-background/backgr_layers-opacity.html a=backout
Link to the failure logs https://treeherder.mozilla.org/logviewer.html#?job_id=161620792&repo=mozilla-inbound&lineNumber=2385
Backout revision cb9b9f2707a3
Failing push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3403b5f669d1395097f26c69c514e52d302a3ec0&filter-searchStr=linux64+opt+Rs8
Status: RESOLVED → REOPENED
status-firefox60: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---

Comment 26

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/8c32e7b0d018
Part 1: Remove mFireAfterPaintEvents and use mTransactions instead. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/7d241f7f2c90
Part 2: nsDisplayMask can paint images and shouldn't have the TYPE_RENDERS_NO_IMAGES flag. r=mstange
https://hg.mozilla.org/mozilla-central/rev/6c8471656b8a
Part 3: Refactor RetainedDisplayListBuilder::AttemptPartialUpdate to have an early return instead of a nested scope. r=miko
https://hg.mozilla.org/mozilla-central/rev/74ecc0facfae
Part 4: Determine when AttemptPartialUpdate made no changes, and return the result to the caller. r=miko
https://hg.mozilla.org/mozilla-central/rev/9e98dcb8a01b
Part 5: Move the Layer building section of nsDisplayList::PaintRoot into a separate function. r=mstange
https://hg.mozilla.org/mozilla-central/rev/dafcb3480048
Part 6: Attempt to skip Layer building if the display list hasn't changed. r=mstange
https://hg.mozilla.org/mozilla-central/rev/85c8836573c5
Part 7: Restrict dirty regions in ComputeRebuildRegion to the overflow area of the current frame so that we discard invalidations that aren't visible. r=miko
Relanded because of mass reftest failures after the latest merge from inbound to central and back to integration trees.

Updated

a year ago
Depends on: 1437392
status-firefox59: affected → wontfix
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.