Closed Bug 1429932 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Web Painting, enhancement, P2)

Product:
Core
Component:
Web Painting
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Bug 1429932 - Part 1: Remove mFireAfterPaintEvents and use mTransactions instead.
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1429932 - Part 2: nsDisplayMask can paint images and shouldn't have the TYPE_RENDERS_NO_IMAGES flag.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1429932 - Part 3: Refactor RetainedDisplayListBuilder::AttemptPartialUpdate to have an early return instead of a nested scope.
59 bytes, text/x-review-board-request
mikokm
: review+
Details
Bug 1429932 - Part 4: Determine when AttemptPartialUpdate made no changes, and return the result to the caller.
59 bytes, text/x-review-board-request
mikokm
: review+
Details
Bug 1429932 - Part 5: Move the Layer building section of nsDisplayList::PaintRoot into a separate function.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1429932 - Part 6: Attempt to skip Layer building if the display list hasn't changed.
59 bytes, text/x-review-board-request
mstange
: review+
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.
59 bytes, text/x-review-board-request
mikokm
: review+
Details 
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 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 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 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 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 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 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 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 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 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 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+
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)
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
Depends on: 1437374
Depends on: 1437436
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 → ---
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.
Depends on: 1438739
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.

Attachment

General

Creator:
Created:
Updated:
Size: