Removing a DOM node with large text contents is slow
Categories
(Core :: Layout, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr115 | --- | unaffected |
firefox127 | --- | wontfix |
firefox128 | --- | wontfix |
firefox129 | --- | fixed |
People
(Reporter: bartveneman, Assigned: TYLin)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(6 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:126.0) Gecko/20100101 Firefox/126.0
Steps to reproduce:
- Render a DOM node on a page with a large string content (300kB of CSS, in my case)
- call
node.remove()
Reduced test case: https://codepen.io/bartveneman/pen/RwmpGej?editors=1010
Actual results:
node.remove()
takes more than 3 seconds to complete. All other browser interactions are blocked in the meantime.
Expected results:
The DOM node is removed instantly and the browser remains responsive to user input.
Comment 1•25 days ago
|
||
The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.
Comment 2•25 days ago
|
||
Can confirm.
Nightly: https://share.firefox.dev/3z726Is
Chrome: https://share.firefox.dev/3yOtQl5 (feels instant)
Comment 3•25 days ago
|
||
Comment 4•25 days ago
|
||
Bisection:
Bug 1865012 Part 4 - Remove PageValuesProperty copy in frame continuations. r=dholbert
It was added in bug 1804772. After Part 1, accessing FirstInFlow() is constant
time, so we don't need to duplicate PageValuesProperty in each frame
continuation.
Differential Revision: https://phabricator.services.mozilla.com/D197759
Updated•25 days ago
|
Comment 5•24 days ago
|
||
Set release status flags based on info from the regressing bug 1865012
Comment 6•24 days ago
|
||
Looks similar to bug 1896875, but it's still slow on Nightly...
Comment 7•24 days ago
|
||
https://searchfox.org/mozilla-central/rev/aa9d148d5be3e7b606448f0b8da6e9f4fa43112f/layout/generic/nsBlockFrame.cpp#6920-6929 seems to be the root cause, we're deleting front to back there.
Assignee | ||
Comment 8•24 days ago
|
||
I'll take a look.
Assignee | ||
Comment 9•23 days ago
•
|
||
Re comment 7:
nsContainerFrame::DeleteNextInFlowChild()
actually collects the next-in-flows in an array, and delete them from back to front in this loop
However, in the testcase (attached in comment 3), the next-in-flows are not in the overflow container list, but in principal child list. The to-be-deleted Inline(code)
has thousands of continuations, so does its child Text
. The partial frame tree looks like
Block(pre)(7)@7f744f2ee148 parent=7f744f2eda20 next=7f744f2ee210 (x=0, y=2010, w=59760, h=8686080) ink-overflow=(x=-180, y=-180, w=274269, h=8686440) scr-overflow=(x=0, y=0, w=274089, h=8686080) [content=7f744b2045e0] [cs=7f744f2e9808] <
line@7f744f2ee598 count=1 state=inline,clean,prevmarginclean,not-impacted,not-wrapped,forced-break-after,clear-before:none,clear-after:none(x=0, y=0, w=0, h=870) <
Inline(code)(0)@7f744f2ee450 parent=7f744f2ee148 next=7f744f2ee6a8 next-in-flow=7f744f2ee6a8 (x=0, y=0, w=0, h=870) [content=7f744b204670] [cs=7f744f235a08] <
Text(0)"\n"@7f744f2ee4f8 parent=7f744f2ee450 next-in-flow=7f744f2ee5e8 (x=0, y=0, w=0, h=870) [content=7f744b205b00] [cs=7f744f235608:-moz-text] [run=7f744b1f2820][0,1,F]
>
>
line@7f744f2ee750 count=1 state=inline,clean,prevmarginclean,not-impacted,not-wrapped,forced-break-after,clear-before:none,clear-after:none(x=0, y=870, w=7361, h=870) <
Inline(code)(0)@7f744f2ee6a8 parent=7f744f2ee148 next=7f744f2ee850 prev-in-flow=7f744f2ee450 next-in-flow=7f744f2ee850 (x=0, y=870, w=7361, h=870) [content=7f744b204670] [cs=7f744f235a08] <
Text(0)".position-unset {\n"@7f744f2ee5e8 parent=7f744f2ee6a8 prev-in-flow=7f744f2ee4f8 next-in-flow=7f744f2ee7a0 (x=0, y=0, w=7361, h=870) [content=7f744b205b00] [cs=7f744f235608:-moz-text] [run=7f744b1ab940][1,18,F]
>
>
line@7f744f2ee8f8 count=1 state=inline,clean,prevmarginclean,not-impacted,not-wrapped,forced-break-after,clear-before:none,clear-after:none(x=0, y=1740, w=10392, h=870) <
Inline(code)(0)@7f744f2ee850 parent=7f744f2ee148 next=7f744f2ee9f8 prev-in-flow=7f744f2ee6a8 next-in-flow=7f744f2ee9f8 (x=0, y=1740, w=10392, h=870) [content=7f744b204670] [cs=7f744f235a08] <
Text(0)"\tposition: unset;\n"@7f744f2ee7a0 parent=7f744f2ee850 prev-in-flow=7f744f2ee5e8 next-in-flow=7f744f2ee948 (x=0, y=0, w=10392, h=870) [content=7f744b205b00] [cs=7f744f235608:-moz-text] [run=7f744b1aba10][19,18,F]
>
>
We are delete aDeletedFrame
, i.e. Inline(code)
front to back repeatedly in this loop. We are also updating the first continuation caches in both the to-be-removed inline frames and their child text frames, which is unnecessary.
nsBlockFrame::DoRemoveFrame()
is quite complex as it deletes continuations while updating/removing lines. At first glance, it doesn’t seem straightforward to rewrite it to delete continuations and lines from back to front. Perhaps its time to introduce a flag in SetPrevContinuation
and SetPrevNextInFlow
to allow stalled cache when we are removing all continuations?
Assignee | ||
Comment 10•22 days ago
|
||
Remove obsolete documentation about "width" being "inline-sizes," because we've
renamed methods to use inline size such as MarkIntrinsicISizesDirty(),
GetPrefISize`, etc.
Remove public:
access specifiers because the sections is already public.
In nsSplittableFrame
, move the warnings to SetPrevContinuation()
and
SetPrevInFlow()
because we update the caches in these two methods, not in
SetNextContinuation()
and SetNextInFlow()
.
Assignee | ||
Comment 11•22 days ago
|
||
Assignee | ||
Comment 12•22 days ago
|
||
The correctness assumption for this patch is that we destroy all frame
continuations when removing an element, so any stale first continuation cache
during frame destruction is acceptable.
Assignee | ||
Comment 13•21 days ago
|
||
Comment 14•18 days ago
|
||
Pushed by aethanyc@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ca817ef0bc6e Part 1 - Revise documentation in nsIFrame and nsSplittableFrame. r=layout-reviewers,emilio https://hg.mozilla.org/integration/autoland/rev/e70f430e9826 Part 2 - Use MOZ_ASSERT_UNREACHABLE() instead of MOZ_ASSERT(false). r=layout-reviewers,emilio https://hg.mozilla.org/integration/autoland/rev/8ea9b9da6922 Part 3 - Add methods to set previous continuation without updating first continuation cache. r=layout-reviewers,dholbert https://hg.mozilla.org/integration/autoland/rev/f1a42b843f2d Part 4 - Add a perf test that removes a child with lengthy text. r=perftest-reviewers,dholbert,sparky
Comment 15•18 days ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ca817ef0bc6e
https://hg.mozilla.org/mozilla-central/rev/e70f430e9826
https://hg.mozilla.org/mozilla-central/rev/8ea9b9da6922
https://hg.mozilla.org/mozilla-central/rev/f1a42b843f2d
Comment 16•18 days ago
|
||
The patch landed in nightly and beta is affected.
:TYLin, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox128
towontfix
.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 17•17 days ago
|
||
Let's let this ride the train because we've shipped this regression for several releases and the patch stack is not trivial.
Description
•