1797011 - Make nsFrameList and AbsoluteFrameList be move-only

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

This is a follow up of Bug 1641085 Part 1. That is, I'd like to get rid of nsFrameList's copy constructor and copy assignment operator so that the frames' ownership and ownership transfer is clearer.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1797011 Part 1 - Delete nsFrameList's copy constructor. r?#layout

This patch doesn't change behavior, and eliminates copy construction of
nsFrameList via utilizing const-references to store the return value of
GetChildList(). This is the first step toward making nsFrameList a move-only
class.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1797011 Part 2 - Make AbsoluteFrameList a move-only class; improve nsFrameConstructorSaveState and PushAbsoluteContainingBlock(). r?#layout

This patch doesn't change behavior.

AbsoluteFrameList is a derived class of nsFrameList, so we change it a move-only
class in order to nsFrameList move-only in Part 3.

Some detail of this patch:

• Define move constructor and move assignment operator for AbsoluteFrameList.
  This effectively disables the auto generated copy constructor and copy
  assignement operator.

• Initialize nsFrameConstructorSaveState's member variables in class definition,
  and remove its constructor.

• Remove mSavedFixedList since we can rewire the logic in
  ~nsFrameConstructorSaveState and PushAbsoluteContainingBlock() to make it
  redundant.

• Make self-assignment correct in nsFrameList's move assignment operator.

Depends on D160013

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1797011 Part 3 - Delete nsFrameList's copy assigment operator. r?#layout

This patch doesn't change behavior.

Depends on D160014

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1797011 Part 4 - Rename AbsoluteFrameList::containingBlock to mContainingBlock to match coding style. r?emilio,#layout

Depends on D160015

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a3f104b78c5
Part 1 - Delete nsFrameList's copy constructor. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/360739dfb6d1
Part 2 - Make AbsoluteFrameList a move-only class; improve nsFrameConstructorSaveState and PushAbsoluteContainingBlock(). r=emilio
https://hg.mozilla.org/integration/autoland/rev/515757be2f7e
Part 3 - Delete nsFrameList's copy assigment operator. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e55bef9aa488
Part 4 - Rename AbsoluteFrameList::containingBlock to mContainingBlock to match coding style. r=emilio,layout-reviewers

Comment 6

[Atila Butkovits]

Backed out for causing high frequency ThreadSanitizer failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/d1b7ddc7c3b5c4951354838afe3e9475533436fe

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=e55bef9aa4881d46e17b9c250ae9440eb0d0e53a&selectedTaskRun=DnEV3gXgRemM4nQhyA7xaQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=394182599&repo=autoland&lineNumber=3003

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18e0c89e758c
Part 1 - Delete nsFrameList's copy constructor. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/3f443cbb5bdd
Part 2 - Make AbsoluteFrameList a move-only class; improve nsFrameConstructorSaveState and PushAbsoluteContainingBlock(). r=emilio
https://hg.mozilla.org/integration/autoland/rev/800047b26683
Part 3 - Delete nsFrameList's copy assigment operator. r=emilio
https://hg.mozilla.org/integration/autoland/rev/59a4b2b70632
Part 4 - Rename AbsoluteFrameList::containingBlock to mContainingBlock to match coding style. r=emilio,layout-reviewers

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/18e0c89e758c
https://hg.mozilla.org/mozilla-central/rev/3f443cbb5bdd
https://hg.mozilla.org/mozilla-central/rev/800047b26683
https://hg.mozilla.org/mozilla-central/rev/59a4b2b70632

Comment 9

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Pulsebot from comment #5)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a3f104b78c5
Part 1 - Delete nsFrameList's copy constructor. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/360739dfb6d1
Part 2 - Make AbsoluteFrameList a move-only class; improve
nsFrameConstructorSaveState and PushAbsoluteContainingBlock(). r=emilio
https://hg.mozilla.org/integration/autoland/rev/515757be2f7e
Part 3 - Delete nsFrameList's copy assigment operator. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e55bef9aa488
Part 4 - Rename AbsoluteFrameList::containingBlock to mContainingBlock to
match coding style. r=emilio,layout-reviewers

== Change summary for alert #35821 (as of Thu, 27 Oct 2022 15:00:44 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
7%	imgur ContentfulSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	1,077.58 -> 1,154.67
7%	amazon ContentfulSpeedIndex	windows10-64-shippable-qr	bytecode-cached fission warm webrender	308.48 -> 329.83

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
11%	amazon loadtime	linux1804-64-shippable-qr	bytecode-cached cold fission webrender	1,147.25 -> 1,017.71
9%	imdb fcp	linux1804-64-shippable-qr	cold fission webrender	630.00 -> 576.04
8%	amazon SpeedIndex	windows10-64-shippable-qr	bytecode-cached cold fission webrender	558.67 -> 514.92
6%	imdb SpeedIndex	linux1804-64-shippable-qr	cold fission webrender	896.24 -> 842.83
4%	buzzfeed LastVisualChange	macosx1015-64-shippable-qr	cold fission webrender	1,090.00 -> 1,043.33
4%	imdb PerceptualSpeedIndex	linux1804-64-shippable-qr	cold fission webrender	1,255.48 -> 1,209.58

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35821