1526972 - MergeState::HasModifiedFrame() is slow

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement, P2)

Description

[Miko Mynttinen]

This function calls |AnyContentAncestorModified()|, which walks up the frame tree until a modified frame or viewport frame is encountered.

We should be able to avoid this call at least for new items1, if we track the modified ancestor state during display list building.

Comment 1

[u480271]

I have a prototype working where I stash a flag in the newly created nsDisplayItem if it the DL builder InInvalidSubtree() is true.

Miko, how are you testing the perf of HasModifiedFrame()?

Comment 2

[Miko Mynttinen]

(In reply to Dan Glastonbury (:kamidphish) | needinfo me from comment #1)

I have a prototype working where I stash a flag in the newly created nsDisplayItem if it the DL builder InInvalidSubtree() is true.

Miko, how are you testing the perf of HasModifiedFrame()?

One way to test it is to open a modified version of our DL mutate test and use a profiler. Modified because the default version does not really present the real-world scenario, where frame trees are much deeper.

On my MBP, profiling the attached dl-test-modified.html?count=20000 shows that 4.5% of total time is spent in AnyContentAncestorModified().
To make it more visible, I annotated the function with MOZ_NEVER_INLINE and in XCode Instruments charged GetParentOrPlaceholderForCrossDoc() to callers.

24.54 s  100.0%	0 s	 	plugin-container (8271)
15.72 s   64.0%	0 s	 	 Main Thread  0xd8ed72
2.42 s    9.8%	2.42 s	 	  nsDisplayBackgroundColor::CreateWebRenderCommands()
1.78 s    7.2%	1.78 s	 	  bincode::internal::serialize_into::h7d2e086eaad43a73
1.11 s    4.5%	1.11 s	 	  AnyContentAncestorModified(nsIFrame*, nsIFrame*)

Matt also had an idea that might be a better solution, since it can be done during display list merging, rather than building:
<mattwoodrow> Currently it walks up to the cross-doc root frame
<mattwoodrow> but it should only need to go up to the frame that owns the current list
<mattwoodrow> since if that was invalid, then we wouldn't be merging the current list, we'd have dumped the whole thing
<mattwoodrow> Assuming that the frame owning a list is a content ancestor of all frames in the list (which I think is true, but maybe there's weird cases)

Comment 3

[Miko Mynttinen]

Attachment: dl-test-modified.html

Comment 4

[u480271]

Attachment: Bug 1526972 - P1: Compress bool state into bit flags.

Also move to first cache-line (64-bytes) of nsDisplayItem to improve D-cache hit
when accessing mFrame, mItemFlags, etc.

Comment 5

[u480271]

Attachment: Bug 1526972 - P2: Re-order mItem->CanBeReused()/mItem->HasDeletedFrame().

To short-circuit the expensive call with a flag check.

Depends on D24459

Comment 6

[u480271]

Attachment: Bug 1526972 - P3: De-virtualize HasDeletedFrame().

To avoid expensive virtual dispatch in PreProcessDisplayList().

Depends on D24460

Comment 7

[u480271]

Attachment: Bug 1526972 - P4: Limit AnyContentAncestorModified frame walk to frame of outer item.

Don't walk frame tree all the way to the root.

Depends on D24461

Comment 8

[u480271]

Attachment: Bug 1526972 - P5: Avoid HasModifiedFrame check for new nsDisplayItems.

aBuilder->InInvalidSubtree() tracks the modified state. Save the state
during construction of nsDisplayItem and use in ProcessItemFromNewList.

Depends on D24462

Comment 9

[u480271]

Attachment: Bug 1526972 - P6: Mark invalidated SubDocument frame as modified.

Depends on D24463

Comment 10

[u480271]

Attachment: Bug 1526972 - P7: Reset InInvalidSubtree when processing unrelated frames.

Depends on D25348

Comment 11

[u480271]

Attachment: Bug 1526972 - P1: Compress bool state into bit flags.

Also move to first cache-line (64-bytes) of nsDisplayItem to improve D-cache hit
when accessing mFrame, mItemFlags, etc.

Comment 12

[u480271]

Attachment: Bug 1526972 - P2: Re-order mItem->CanBeReused()/mItem->HasDeletedFrame().

To short-circuit the expensive call with a flag check.

Depends on D24459

Comment 13

[u480271]

Attachment: Bug 1526972 - P3: De-virtualize HasDeletedFrame().

To avoid expensive virtual dispatch in PreProcessDisplayList().

Depends on D24460

Comment 14

[u480271]

Attachment: Bug 1526972 - P4: Limit AnyContentAncestorModified frame walk to frame of outer item.

Don't walk frame tree all the way to the root.

Depends on D24461

Comment 15

[u480271]

Attachment: Bug 1526972 - P5: Avoid HasModifiedFrame check for new nsDisplayItems.

aBuilder->InInvalidSubtree() tracks the modified state. Save the state
during construction of nsDisplayItem and use in ProcessItemFromNewList.

Depends on D24462

Comment 16

[u480271]

Attachment: Bug 1526972 - P6: Mark invalidated SubDocument frame as modified.

Depends on D24463

Comment 17

[u480271]

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3000884569814d22e3decda8a28cf2c25a474f8

I think the diagnostic asserts are finally fixed!

Comment 18

[u480271]

Attachment: Bug 1526972 - P7: Move AutoBuildingDisplayList constructor into .cpp

Comment 19

[u480271]

Attachment: Bug 1526972 - P8: Reset InInvalidSubtree when processing unrelated frames.

Comment 20

[u480271]

Try run looks pretty good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ded024ff14f9aa8ff89f4fab5664d873508b4f&selectedJob=239852315

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a59f06022a95
P1: Compress bool state into bit flags. r=miko
https://hg.mozilla.org/integration/autoland/rev/54b14df56e6f
P2: Re-order mItem->CanBeReused()/mItem->HasDeletedFrame(). r=miko
https://hg.mozilla.org/integration/autoland/rev/ddd57e437228
P3: De-virtualize HasDeletedFrame(). r=miko
https://hg.mozilla.org/integration/autoland/rev/8543b9d46521
P4: Limit AnyContentAncestorModified frame walk to frame of outer item. r=miko
https://hg.mozilla.org/integration/autoland/rev/2fb940b13971
P5: Avoid HasModifiedFrame check for new nsDisplayItems. r=miko
https://hg.mozilla.org/integration/autoland/rev/2ea2f8533078
P6: Mark invalidated SubDocument frame as modified. r=miko,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/a895c9028b31
P7: Move AutoBuildingDisplayList constructor into .cpp r=miko
https://hg.mozilla.org/integration/autoland/rev/815543d81a1d
P8: Reset InInvalidSubtree when processing unrelated frames. r=mattwoodrow

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a59f06022a95
https://hg.mozilla.org/mozilla-central/rev/54b14df56e6f
https://hg.mozilla.org/mozilla-central/rev/ddd57e437228
https://hg.mozilla.org/mozilla-central/rev/8543b9d46521
https://hg.mozilla.org/mozilla-central/rev/2fb940b13971
https://hg.mozilla.org/mozilla-central/rev/2ea2f8533078
https://hg.mozilla.org/mozilla-central/rev/a895c9028b31
https://hg.mozilla.org/mozilla-central/rev/815543d81a1d

Comment 23

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a59f06022a95
https://hg.mozilla.org/mozilla-central/rev/54b14df56e6f
https://hg.mozilla.org/mozilla-central/rev/ddd57e437228
https://hg.mozilla.org/mozilla-central/rev/8543b9d46521
https://hg.mozilla.org/mozilla-central/rev/2fb940b13971
https://hg.mozilla.org/mozilla-central/rev/2ea2f8533078
https://hg.mozilla.org/mozilla-central/rev/a895c9028b31
https://hg.mozilla.org/mozilla-central/rev/815543d81a1d

Comment 24

[Natalia Csoregi [:nataliaCs]]

backout: https://hg.mozilla.org/mozilla-central/rev/9f0f38c38ccce7c9656c77205b16dfb35208df2d

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55d7bb4de1ab
P1: Compress bool state into bit flags. r=miko
https://hg.mozilla.org/integration/autoland/rev/5aa53da6778d
P2: Re-order mItem->CanBeReused()/mItem->HasDeletedFrame(). r=miko
https://hg.mozilla.org/integration/autoland/rev/5ef6aebe5737
P3: De-virtualize HasDeletedFrame(). r=miko
https://hg.mozilla.org/integration/autoland/rev/4df706db804e
P4: Limit AnyContentAncestorModified frame walk to frame of outer item. r=miko
https://hg.mozilla.org/integration/autoland/rev/2459a266d318
P5: Avoid HasModifiedFrame check for new nsDisplayItems. r=miko
https://hg.mozilla.org/integration/autoland/rev/4e0d56e43a73
P6: Mark invalidated SubDocument frame as modified. r=miko,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/069c60c771c6
P7: Move AutoBuildingDisplayList constructor into .cpp r=miko
https://hg.mozilla.org/integration/autoland/rev/817642dc917c
P8: Reset InInvalidSubtree when processing unrelated frames. r=mattwoodrow

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/55d7bb4de1ab
https://hg.mozilla.org/mozilla-central/rev/5aa53da6778d
https://hg.mozilla.org/mozilla-central/rev/5ef6aebe5737
https://hg.mozilla.org/mozilla-central/rev/4df706db804e
https://hg.mozilla.org/mozilla-central/rev/2459a266d318
https://hg.mozilla.org/mozilla-central/rev/4e0d56e43a73
https://hg.mozilla.org/mozilla-central/rev/069c60c771c6
https://hg.mozilla.org/mozilla-central/rev/817642dc917c