1682556 - 15.95 - 29.8% perf_reftest_singletons inline-style-cache-1.html / perf_reftest_singletons link-style-cache-1.html (macosx1014-64-shippable-qr) regression on push 8ba485d9c77b9c97819197b7b8ae6738ea68e380 (Thu December 10 2020)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

Perfherder has detected a talos performance regression from push 8ba485d9c77b9c97819197b7b8ae6738ea68e380. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
30%	perf_reftest_singletons	link-style-cache-1.html	macosx1014-64-shippable-qr	e10s stylo webrender	1,272.08 -> 1,651.20
29%	perf_reftest_singletons	link-style-cache-1.html	macosx1014-64-shippable-qr	e10s stylo webrender	1,273.46 -> 1,646.89
16%	perf_reftest_singletons	inline-style-cache-1.html	macosx1014-64-shippable-qr	e10s stylo webrender	2,043.67 -> 2,369.71

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Will investigate. Is this really mac-only?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Hmm, it sure looks like it: https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&series=autoland,2847199,1,1&series=mozilla-central,2112065,1,1&series=mozilla-central,2112069,1,1&series=mozilla-central,2819818,1,1&series=mozilla-central,2818996,1,1&series=mozilla-central,2819933,1,1&timerange=1209600

Maybe pages_copy is somehow much slower than memcpy? Mike, do you know why mac uses pages_copy instead of memcpy? It goes back to bug 414946 but I don't understand why it'd be needed.

Will double-check if that's the culprit though. I think that has to be the culprit since more regular benchmarks where we presumably don't go over the page limit like stylebench didn't regress, and this test-case allocates a giant smallvec because it basically creates 30k matching rules.

Comment 3

[Mike Hommey [:glandium]]

IIRC vm_copy is supposed to not be an actual copy, so it should actually be faster. That is, it is sort of like mremap, but with preservation of the original mapping, and making both mappings independent as far as modifications to each of them go.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Seems to be it: https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=d489921f5f851536aa4918abaf2eaf2010e32f06&originalSignature=2821995&newSignature=2828796&framework=1&selectedTimeRange=172800

Will see what it does to the rest of our perf tests, but seems worth a shot trying to get it landed if it doesn't regress other stuff.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1682556 - Remove pages_copy realloc() special case, as it seems slower than memcpy() in micro-benchmarks. r=glandium

Will do a more exhaustive perf run with this, but this seems to fix the
regression and, well, yay for less special-cases I guess.

Comment 6

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

Set release status flags based on info from the regressing bug 1632469

Comment 7

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

Seems like somebody asked the question that was addressed to me.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

• Baseline vs. patch: https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=71f6ce34fa53d25e4aba7a469ed5e9ce6a1b8bc7&originalSignature=2828796&newSignature=2828796&framework=1&originalRevision=2a8c6c8589cfd85f72844d1d2df4ee308a39c786
• Baseline vs. Glandium's suggestion: https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=bc272d1a6ae31ce864d77599cfe9c58b23140f16&originalSignature=2828796&newSignature=2828796&framework=1&originalRevision=2a8c6c8589cfd85f72844d1d2df4ee308a39c786 (also fixes the regression)

So either way works for me, your call. I guess ideally we need a benchmark that improves by using vm_copy, but if you want to just increase the threshold to be the chunk size for now that's fine for me.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1682556 - Increase vm_copy() threshold. r=glandium

The current implementation is a regression on microbenchmarks that
reallocate allocations that go over gPageSize * 32 compared to memcpy().

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4a2022ff76d
Increase vm_copy() threshold. r=glandium

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/b4a2022ff76d

Comment 12

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

I think it's ok for this to ride the trains.

Comment 14

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

== Change summary for alert #28265 (as of Tue, 29 Dec 2020 04:43:04 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
23%	perf_reftest_singletons	link-style-cache-1.html	macosx1014-64-shippable-qr	e10s stylo webrender	1,644.98 -> 1,268.55
13%	perf_reftest_singletons	inline-style-cache-1.html	macosx1014-64-shippable-qr	e10s stylo webrender	2,369.44 -> 2,063.44
11%	displaylist_mutate		macosx1014-64-shippable-qr	e10s stylo webrender	4,254.16 -> 3,787.85

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