Closed Bug 1633410 Opened 3 months ago Closed 25 days ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox-esr68 --- unaffected
firefox75 --- unaffected
firefox76 --- unaffected
firefox77 --- fixed
firefox78 + fixed
firefox79 --- fixed

People

(Reporter: marauder, Assigned: SimonSapin)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: perf, perf-alert, regression)

Attachments

(8 files)

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.

Blocks: 1628613
Component: Performance → CSS Parsing and Computation
Flags: needinfo?(simon.sapin)
Product: Testing → Core
Regressed by: 1631721
Version: Version 3 → unspecified

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?

Priority: -- → P2

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

Thanks in advance.

Flags: needinfo?(marian.raiciof)

(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.

Flags: needinfo?(marian.raiciof)

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

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

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?).

Flags: needinfo?(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.

Flags: needinfo?(tnikkel)

Hi Emilio, are you the assignee here?

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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?

Flags: needinfo?(simon.sapin)

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

Flags: needinfo?(simon.sapin)

This reverts commit 62cfd95c4f71cecfff0dbfef31ecdec599095549.

This reverts commit 6ef0925c21b0c7529a7b8786981db10eb5dad673.

This reverts commit 64ad40e79dd94fa742c8757f09cfe131ad9829c9.

This reverts commit 198028f6492ceac1d2e708b5f07bab72933d6f42.

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

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

Flags: needinfo?(simon.sapin)
Assignee: nobody → simon.sapin
Status: NEW → ASSIGNED

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.

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(dholbert)

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

(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.

Severity: -- → S2
Flags: needinfo?(dholbert)

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

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
Attachment #9149279 - Flags: approval-mozilla-beta?
Attachment #9149280 - Flags: approval-mozilla-beta?
Attachment #9149281 - Flags: approval-mozilla-beta?
Attachment #9149282 - Flags: approval-mozilla-beta?

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

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

Attachment #9149279 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9149280 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9149281 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9149282 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

Regression is still on Nightly.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

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.

Flags: needinfo?(pascalc)

Forwarding the NI to Julien who owns 78

Flags: needinfo?(pascalc) → needinfo?(jcristau)
Attachment #9154010 - Flags: approval-mozilla-beta+
Status: REOPENED → RESOLVED
Closed: 2 months ago25 days ago
Resolution: --- → FIXED
Target Milestone: mozilla77 → mozilla79

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

You need to log in before you can comment on or make changes to this bug.