1817078 - PresShell::mAllocatedPointers handling shows up pretty badly in some profiles (on Nightly)

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

I was looking at some profiles and noticed that adding, accessing and removing objects from the hashtable was more than 10% of the total time spent in layout.
Has the hashtable proved to be useful? Could it be debug only?

Or perhaps we just should use nightly-as-release builds for profiling, once one can download them easily.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug is a defect, but please change it back in case of error.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

This doesn't need to be tracked as regression.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #0)

Has the hashtable proved to be useful?

Occasionally, I think. We do have open bugs filed for failures here. Bug 1794352 in particular is filed on in-the-wild cases where the hashtable's associated assertion is getting tripped. We also have bug 1704908 for a fuzzer bug that seems to be pointing at a leak (items in the hashtable not being freed).

Could it be debug only?

For fuzzing purposes (bug 1704908), it could be. For in-the-wild cases (bug 1794352), we'd lose the ability to detect these potential use-after-free cases.

Or perhaps we just should use nightly-as-release builds for profiling, once one can download them easily.

That seems like a good idea for lots of reasons, to better capture the experience of release users...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I think it's probably ok to make these debug-only fwiw. I'll send a patch tomorrow if nobody beats me to it. Bug 1794352 etc might be bitflips, specially if they happen in pages like youtube and so on which are hit very frequently by users.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1817078 - Make allocated pointer tracking DEBUG-only. r=dholbert,jfkthame

It's helpful to catch issues, but I don't think we get much value in
return in practice.

Comment 6

[Daniel Holbert [:dholbert]]

Yeah, to the extent that we're willing to write off Bug 1794352 as bitflips & hardware issues[1], that sounds fine I guess.

[1] (which is probably reasonable given volume & high-profile nature of the sites & the crash-volume we would expect to get if those sites were actually triggering some layout bug),

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d5007b719e4
Make allocated pointer tracking DEBUG-only. r=dholbert

Comment 8

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/7d5007b719e4

Comment 9

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #7)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d5007b719e4
Make allocated pointer tracking DEBUG-only. r=dholbert

== Change summary for alert #37243 (as of Mon, 20 Feb 2023 06:18:16 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
34%	perf_reftest_singletons style-attr-1.html	linux1804-64-shippable-qr	e10s fission stylo webrender	6.08 -> 4.01
12%	perf_reftest_singletons style-sharing.html	windows10-64-shippable-qr	e10s fission stylo webrender	4.77 -> 4.22

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