1633410 - 3.32 - 4.37% Images (linux1804-64-shippable) regression on push d0ee684793c43c000d185be4b5d9f7f4e1f8f483 (Thu April 23 2020)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Marian Raiciof [:marauder]]

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

Regressions:

4% Images linux1804-64-shippable opt tp6 8,441,484.65 -> 8,810,037.15
3% Images linux1804-64-shippable opt tp6 8,472,711.74 -> 8,762,310.99
3% Images linux1804-64-shippable opt 5,572,978.87 -> 5,758,279.55

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]]

So this is a memory usage regression, and it seems consistent (so it didn't recover)... Maybe hashbrown is less memory-efficient than the old std hashmap that hashbrown was based on?

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Marian, what is this test measuring? Total memory usage? Unclassified memory? Something else?

Thanks in advance.

Comment 3

[Eric Rahm [:erahm]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Marian, what is this test measuring? Total memory usage? Unclassified memory? Something else?

Thanks in advance.

It looks like this regressed both the standard awsy test and the awsy-tp6 variant. For the tp6 variant it simulates a heavy user session by loading each page of the tp6 pageset in a separate tab. It takes about 11 minutes to run, you can run it locally with:

./mach awsy-test --tp6 --preferences testing/awsy/conf/tp6-prefs.json

The regressions noted here are for the about:memory explicit/images measurement.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1633410 - Shrink cascade data after rebuild.

I thought this would've helped, but seems neutral. Anyhow didn't want to
lose the patch :(

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1633410 - Don't underreport memory for cleared maps.

This one I haven't run yet, but not sure it'd help given comment 3.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

So I'm a bit confused. If this is only for explicit/images I can't see how this could've been caused by bug 1631721.

Timothy do you know how is explicit/images measured? The only plausible explanation is that that measurement somehow includes style system measurements (maybe for SVG images or such?).

Comment 7

[Timothy Nikkel (:tnikkel)]

VectorImage::SizeOfSourceWithComputedFallback eventually calls Document::DocAddSizeOfExcludingThis which adds in the size of the style set. It seems reasonable that that computed value would end up in explicit/images but I didn't follow the path.

Comment 8

[Rachel Tublitz [:rachel]]

Hi Emilio, are you the assignee here?

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Well, ideally Simon could dig a bit more, as he wrote the regressing patch. But...

Comment 10

[Simon Sapin (:SimonSapin)]

Sorry for the late response. I don’t really know where to start investigating this. It may be best to back out bug 1631721 until this regression can be solved. How does that work? Should I open a new bug with patches that revert the initial ones?

Comment 11

[Julien Cristau [:jcristau]]

Either that or post them here, yes. They probably don't back out cleanly at this point?

Comment 12

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1633410 - Revert "Bug 1631721 - Remove hashglobe"

This reverts commit 62cfd95c4f71cecfff0dbfef31ecdec599095549.

Comment 13

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1633410 - Revert "Bug 1631721 - Use std::alloc instead of hashbrown::alloc in fallible"

This reverts commit 6ef0925c21b0c7529a7b8786981db10eb5dad673.

Comment 14

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1633410 - Revert "Bug 1631721 - Use hashbrown instead of hashglobe"

This reverts commit 64ad40e79dd94fa742c8757f09cfe131ad9829c9.

Comment 15

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1633410 - Revert "Bug 1631721 - Vendor the hashbrown crate"

This reverts commit 198028f6492ceac1d2e708b5f07bab72933d6f42.

Revert note: most of the commit is not reverted, since mp4parse since then started depending on hashbrown as well.

Comment 16

[Simon Sapin (:SimonSapin)]

Added four back-out patches. Hashes in "This reverts commit […]" messages are those of git-cinnabar.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Hold on, I'm not sure we should back them out, fwiw. This is a linux-only regression, which was also met with a linux-only perf improvement in the regressing bug...

So while I think we should try to mitigate the regression, I don't think we should go back to having our own fork of std hashmap for this.

Comment 18

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

The severity field is not set for this bug.
:dholbert, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 19

[Manish Goregaokar [:manishearth]]

r+ but we should wait and be sure we really want to back this out

Comment 20

[Eric Rahm [:erahm]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

Hold on, I'm not sure we should back them out, fwiw. This is a linux-only regression, which was also met with a linux-only perf improvement in the regressing bug...

Looking at the graphs it was across the board. It's primarily on the "open but settled" and "closed" metrics, so we have more memory sitting around after various GC/CC things run and when we close tabs.

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Ok, let's back this out from beta, I want to investigate a bit more.

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9149279 [details]
Bug 1633410 - Revert "Bug 1631721 - Remove hashglobe"

Beta/Release Uplift Approval Request

• User impact if declined: Memory regression.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Back out of patch that introduced memory regression.
• String changes made/needed: none

Comment 23

[Pascal Chevrel:pascalc]

Comment on attachment 9149279 [details]
Bug 1633410 - Revert "Bug 1631721 - Remove hashglobe"

Fix for a memory regression, uplift approved for beta, thanks.

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7db1ef25a39b
https://hg.mozilla.org/releases/mozilla-beta/rev/f60180d58faa
https://hg.mozilla.org/releases/mozilla-beta/rev/53621ed25d50
https://hg.mozilla.org/releases/mozilla-beta/rev/d1a9d96c3989

Comment 25

[Emilio Cobos Álvarez [:emilio]]

Regression is still on Nightly.

Comment 26

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1633410 - Revendor Rust dependencies, removing Hashbrown

Comment 27

[Simon Sapin (:SimonSapin)]

Beta is busted because building causes Cargo.lock to change: https://treeherder.mozilla.org/logviewer.html#?job_id=302920038&repo=mozilla-beta

This is because I made the revert patches against central, where mp4parse added its own dependency to Hashbrown, so I removed the part of the revert commit that removes third_party/rust/hashbrown. But that mp4parse change is not in beta yet, so third_party/rust/hashbrown should indeed be removed. Running ./mach vendor rust automatically fixes that and Cargo.lock. This last patch does so against beta.

Unfortunately this situation will cause the next merge from central to beta to either conflict or fail to build, because mp4parse will start relying on third_party/rust/hashbrown which has been removed. Running ./mach vendor rust again then should automatically fix this.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Rust revendor uplift: https://hg.mozilla.org/releases/mozilla-beta/rev/def0b5d7a9aebbb9259de16b164740f51fceb2f9

Comment 29

[Emilio Cobos Álvarez [:emilio]]

Can we get this in beta78 as well? I haven't been able to dig a lot into this so I'll land it in Nightly too for now.

Comment 30

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1633410 - Back out bug 1631721.

Rebased and squashed.

Comment 31

[Pascal Chevrel:pascalc]

Forwarding the NI to Julien who owns 78

Comment 32

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ba29749e871

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1128a5a77dd
Back out bug 1631721.

Comment 34

[Cristina Coroiu [:ccoroiu]]

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

Comment 35

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

== Change summary for alert #26278 (as of Fri, 19 Jun 2020 07:05:19 GMT) ==

Improvements:

14% perf_reftest coalesce-2.html windows10-64-shippable-qr opt e10s stylo 27.48 -> 23.60
10% perf_reftest coalesce-2.html macosx1014-64-shippable opt e10s stylo 43.55 -> 39.02
6% perf_reftest coalesce-1.html windows10-64-shippable-qr opt e10s stylo 63.04 -> 59.03

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26278