Closed Bug 1818157 Opened 3 years ago Closed 3 years ago

2.36 - 1.36% google-slides loadtime / google-slides loadtime (Linux) regression on Thu February 16 2023

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
112 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- unaffected
firefox110 --- unaffected
firefox111 --- unaffected
firefox112 --- fixed

People

(Reporter: aesanu, Unassigned)

References

(Regression)

Details

(Keywords: perf, perf-alert, regression)

Attachments

(1 file)

backout patch to land
26.63 KB, patch
Details | Diff | Splinter Review

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

Regressions:

Ratio Test Platform Options Absolute values (old vs new) Performance Profiles
2% google-slides loadtime linux1804-64-shippable-qr cold fission webrender 6,079.67 -> 6,223.08 Before/After
1% google-slides loadtime linux1804-64-shippable-qr cold fission webrender 6,096.90 -> 6,179.96 Before/After

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) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

For more information on performance sheriffing please see our FAQ.

Flags: needinfo?(longsonr)

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

status-firefox110: --- → unaffected
status-firefox111: --- → unaffected
status-firefox-esr102: --- → unaffected

please can you run the profiling jobs.

Flags: needinfo?(longsonr) → needinfo?(aesanu)

I'm afraid I don't understand how to interpret the information in this bug. What I'd need to know is which method calls are now slower.

Looking at the "Performance Profiles" links from comment 0 (in particular: downloading the files, loading them at https://profiler.firefox.com/ , and choosing the first piece of each one which is browser-cycle-1.json.gz), I get these two profiles:
Before: https://share.firefox.dev/3SoPSke
After: https://share.firefox.dev/41hEWsL

I've zoomed both profiles to the region that seems most relevant, which is from the triggerRender_0 UserTiming mark (in the marker chart) up until the content process quiets down. I've also filtered both profiles for "svg" as a rough measure of "how much time are we spending in doing SVG-related stuff (whether layout, painting, or other code), to get an idea for what bug 1814398 could have realistically changed.

We do have a substantial increase in matching samples -- 691 samples with svg in the backtrace in the "Before" profile, vs 910 in the "after" profile. So, a delta of 219 samples, which represents roughly 219ms of time (since we've got a 1ms sample frequency).

In these particular profiles, this is partially (maybe entirely?) explained by the fact that we ended up having a different number of Restyle/Reflow/Repaint refresh driver ticks... I count 18 ticks in the "before" profile vs. 23 in the "After" profile. Each tick is around 45ms (mostly occupied by reflow), so this 5-tick-delta with ~45ms per tick can easily account for that 219ms difference.

So my question right now is:

  • Was that refresh driver tick-count delta just "bad luck" in the two profiles in comment 0, or does it persist in other before/after profiles of this benchmark
  • If that delta is "real", what is causing it (and how might bug 1814398 have produced that change)

I'm also not sure if this could be relevant or not, but it looks like this alert is precisely aligned with an "infra change" marker in the graph. If you click the 1% or 2% values in comment 0 and hover the yellow bar near the middle of the graph (which happens to precisely align with the alert), it shows this message about the infrastructure change:
Simple implementation of a test run manager for mozilla tests at bitbar.

I don't know what that means or if it's relevant to this particular testsuite or not... aesanu perhaps you know where to find out more about that?

I did some retriggers and generated some more "gslides-p" profiling tasks, but the variability is quite large, which makes it hard to reason about what the relevant difference might be.

Here's the pushlog with before/after csets and a bunch of gslides and gslides-p tasks:
https://treeherder.mozilla.org/jobs?repo=autoland&fromchange=977500d3fc3cb2838a082c9b714aa1bc2a6af6d1&tochange=510d250fd54522af8d05ea0f64094769d624a87c&searchStr=gslides%2Clinux%2Cwebrender

Looking at the gslides-p tasks, and specifically the reported values for google-slides loadtime opt cold fission gecko-profile webrender there, I'm seeing values that range from 3166 up to 8045.5, for the 'baseline' revision...

One method of investigation that's been helpful to me in the past is pushing to try various reductions of what the patch does to see what the minimal change is that causes the uptick. This would also confirm if it is indeed attributed to the right patch.

This is already pretty minimal, it was somewhat larger but before landing I split parts off into bug 1815959, bug 1816070, bug 1815920 and landed them separately.

The remaining code changes:

  1. SVGImageElement derives from SVGGraphicsElement rather than SVGGeometryElement. This will make SVGImageElement instances marginally smaller.
  2. SVGImageFrame derives from nsIFrame rather than SVGGeometryFrame
  3. Remove a couple of image checks in SVGGeometryFrame
  4. Remove some virtual keywords from SVGImageFrame as they no longer override something in SVGGeometryFrame
  5. Introduce new webrender code (DisplaySVGImage) specifically for SVGImageFrame since it's no longer covered by SVGGeometryFrame. The code is exactly the same for as it was for DisplaySVGGeometry though.
  6. Introduce a new webrender pref for svg-shapes now that svg-images only covers images and no longer covers shapes as well.

It should be faster (though that's really really marginal) because

  1. SVGGeometryFrame now no longer checks for images.
  2. SVGImageFrame has non-virtual methods in many places as it does not need to override SVGGeometryFrame any more

The only thing I can see that might be marginally slower is the introdcution of an AssessBounds function rather than having bounds assessment be inline but surely a non-virtual function call isn't measurably slower here.

For convenience (since it took a few minutes to construct), here's the "Perfherder Compare Revisions" view of the base revision vs. longsonr's SVGImageFrame revision, with "hide uncertain results" and "hide uncomparable results" activated:
https://treeherder.mozilla.org/perfherder/compare?originalProject=autoland&originalRevision=977500d3fc3cb2838a082c9b714aa1bc2a6af6d1&newProject=autoland&newRevision=510d250fd54522af8d05ea0f64094769d624a87c&framework=13&page=1&filter=linux+google-slides+loadtime&showOnlyComparable=1&showOnlyConfident=1

That shows we have ~50 retries of the before and after revisions; and with that data, perfherder has a "high" confidence assessment of a ~1.4% regression in the various artifacts from the google-slides loadtime opt cold fission test.

I've also kicked off a fresh try run of the base revision vs. longsonr's SVGImageFrame revision, to see if this shows up there (and to test the theory of whether this could've been build-config related or something along those lines).

So far, my TryServer experiments are proving consistent with the theory that this did in fact introduce a small perf regression.

Experiment 1 (yesterday): Comparing fresh try builds of bug 1814398's parent commit vs. its own commit
(basically validating "Did the regression occur?")
URL: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=61b9f78ce7750048a13913477c393707e64bfd53&newProject=try&newRevision=35cd3f36045c82443f2530194e7405856997f393&framework=13&page=1&filter=loadtime+cold+webrender
Current data (40 retries on linux, 20-30 retries on other platforms) show a low-confidence 1-2% regression on most metrics, though there's a contradictory (low-confidence) 1% improvement for mac.
Most importantly, if "confidence" scores can be trusted: the data shows a medium-confidence regression from 1-6% for all of of the non-"bytecode-cached" measurements on Windows, and a high-confidence regression of 2% for the "bytecode-cached" measurement on Linux.

Experiment 2 (today): comparing mozilla-central tip, vs. tip with bug 1814398 backed out
(basically validating "Would a backout fix the regression at this point")
URL: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=2e7f0ba2d404fad8ef8c44d2d037341bae0a8cd5&newProject=try&newRevision=fc3a8061a5cbefff2728480e2ab02587a4491010&framework=13&page=1&filter=loadtime+cold+webrender
Current data (10 retries on all platforms) show a 1-2% improvement on most metrics.

So: based on these trials, it seems like the regression was real (and not linux-specific; Windows at least might have a similar regression), and a backout would fix it, and this wasn't related to infrastructure changes (comment 5) since my before/after trials were all using the same infrastructure (pushed back-to-back, with no infra-related cset differences).

The only thing I can think of is to inline the AssessBounds method: https://searchfox.org/mozilla-central/source/gfx/layers/wr/WebRenderCommandBuilder.cpp#1133

removing the call to nsIFrame::AttributeChanged (which is a no-op) in SVGImageFrame::AttributeChanged will increase performance but the patch didn't make that worse because it used to call SVGGeometryFrame so there was a virtual call before the patch (just a different one).

Robert and I are both trying smaller versions of his patch on Try to see if we can find out which part is responsible for the performance regression.

In the meantime, I've requested that sheriffs back out bug 1814398 (and followup bug 1817212); we can re-land an updated patch once we've got a better understanding of what's going on here perf-wise.

Turns out that our regular backout tooling gets slightly tripped up by a backout since one of bug 1814398's changes was already reverted (for bug 1817609 which otherwise seems to be unrelated).

Fortunately hg backout doesn't mind; I'll post a patch that we can use to do the backout, as an attachment here.

Attachment #9320346 - Attachment is patch: true
Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ed8ad44b1a2a Backout changeset 510d250fd545 (bug 1814398) and e44b3ab61ae4 (bug 1817212) for introducing a performance regression. CLOSED TREE

We can let this get auto-closed when the patch merges to central. Assuming the regression was really caused by the blamed changes (which all signs are pointing to so far), we should see a perf improvement associated with comment 17.

ni=me to remind myself to confirm the improvement, once we've gotten the various performance tasks back.

Flags: needinfo?(dholbert)

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

Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox112: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 112 Branch
Assignee: nobody → dholbert
Assignee: dholbert → nobody
Flags: needinfo?(dholbert)

(In reply to Daniel Holbert [:dholbert] from comment #18)

we should see a perf improvement associated with comment 17.

ni=me to remind myself to confirm the improvement, once we've gotten the various performance tasks back.

Perfherder comparison of the backout commit (right) vs. the prior commit (left):
https://treeherder.mozilla.org/perfherder/compare?originalProject=autoland&originalRevision=f898aea0f93751759d77096cc97be4a0b493d5de&newProject=autoland&newRevision=ed8ad44b1a2ab53742a7bb4ac9972016036a3353&framework=13&page=1&filter=linux+google-slides+loadtime&showOnlyComparable=1

No google-slides tasks yet, but I just triggered them, so data should show up at some point today. (I'll probably need to retrigger a few times too, to get enough data to have significance.

(Yeah, that perfherder comparison does indeed seem to show a perf improvement associated with the backout.)

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

Attachment

General

Creator:
Created:
Updated:
Size: