MergeState::HasModifiedFrame() is slow

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: miko, Assigned: kamidphish)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(9 attachments, 7 obsolete attachments)

3.87 KB, text/html
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Reporter

Description

3 months ago

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.

Priority: P1 → P2
Assignee: nobody → dglastonbury

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()?

Flags: needinfo?(mikokm)
Reporter

Comment 2

2 months ago

(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)

Flags: needinfo?(mikokm)
Reporter

Comment 3

2 months ago

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

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

Depends on D24459

To avoid expensive virtual dispatch in PreProcessDisplayList().

Depends on D24460

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

Depends on D24461

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

Depends on D24462

Reporter

Updated

2 months ago
Status: NEW → ASSIGNED

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

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

Depends on D24459

To avoid expensive virtual dispatch in PreProcessDisplayList().

Depends on D24460

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

Depends on D24462

Attachment #9055832 - Attachment is obsolete: true
Attachment #9055831 - Attachment is obsolete: true
Attachment #9055830 - Attachment is obsolete: true
Attachment #9055829 - Attachment is obsolete: true
Attachment #9055828 - Attachment is obsolete: true
Attachment #9055827 - Attachment is obsolete: true
Attachment #9055826 - Attachment is obsolete: true
Attachment #9055825 - Attachment is obsolete: true
Attachment #9054397 - Attachment is obsolete: true
Attachment #9054396 - Attachment is obsolete: true
Attachment #9052776 - Attachment is obsolete: true
Attachment #9052775 - Attachment is obsolete: true
Attachment #9052774 - Attachment is obsolete: true
Attachment #9052773 - Attachment is obsolete: true
Attachment #9052772 - Attachment is obsolete: true
Attachment #9055825 - Attachment is obsolete: false
Attachment #9055826 - Attachment is obsolete: false
Attachment #9055827 - Attachment is obsolete: false
Attachment #9055828 - Attachment is obsolete: false
Attachment #9055829 - Attachment is obsolete: false
Attachment #9055830 - Attachment is obsolete: false
Attachment #9055831 - Attachment is obsolete: false
Attachment #9055832 - Attachment is obsolete: false

Comment 21

a month ago
Pushed by dglastonbury@mozilla.com:
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
Regressions: 1544414
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla68 → ---

Comment 25

a month ago
Pushed by dglastonbury@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.