Closed Bug 1727754 Opened 5 months ago Closed 3 months ago

0.21 - 0.16% installer size / installer size + 1 more (OSX) regression on Tue August 24 2021

Categories

(Core :: Gecko Profiler, defect, P2)

defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox91 --- unaffected
firefox92 --- unaffected
firefox93 --- wontfix
firefox94 --- fix-optional

People

(Reporter: aesanu, Assigned: gerald)

References

(Depends on 1 open bug, Regression)

Details

(Keywords: perf-alert, regression)

Perfherder has detected a build_metrics performance regression from push 12796ed8484e8fd56b5c87d7a11fb0dda3f01492. 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)
0.21% installer size osx-shippable nightly 84,327,447.75 -> 84,505,819.17
0.20% installer size osx-shippable nightly 84,332,314.83 -> 84,498,282.17
0.16% installer size osx-aarch64-shippable aarch64 nightly 81,097,329.67 -> 81,227,003.92

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.

Flags: needinfo?(gsquelart)

That code is an important change in the profiler, it's a replacement of the thread registration classes, with a few advantages:

  • Better ownership model, which should remove some crash situations (e.g., after the profiler and/or a thread ends, but the data is still accessed.)
  • Better thread-safety of the data, with appropriate proof-of-locks, and in some cases no-lock when guaranteed to be accessed on the owning thread.
  • Better mutex granularity, allowing some thread-specific data to be access in parallel, when it was previously tied to a central mutex. This lowers the runtime overhead of the profiler.
  • It allows on-stack thread registration, which will remove heap allocations.
  • Preparing for future changes that will help register yet-unknown threads.

So overall I think it's worth keeping at the cost of less than 200KB.

But I'd be happy to investigate if there are things I could improve. E.g., there is quite a bit of code in headers, which could possibly be moved to cpp files, to potentially reduce redundant generated code.
I can look at it next week, but I can't promise that it can be done for sure.

Please let me know if you need more information.
If you agree to the above, I'll assign myself to the bug.

Flags: needinfo?(gsquelart)

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

I will look at regression bug 1727877 first, it's more important I think (3% regression in runtime memory usage).

Assignee: nobody → gsquelart
Depends on: 1727877
Priority: -- → P2

I've tried to move some code from headers to cpps, but I didn't see useful results, in fact in lots of cases it makes it worse!
Also, I guess shippable and zipped files may not be strictly reproducible, making these results more difficult to judge.

Baseline: https://treeherder.mozilla.org/jobs?repo=try&revision=ea3409c3d2a0f55f8c482d61ca52e7907a0a638d
ThreadRegistrationData changes: https://treeherder.mozilla.org/jobs?repo=try&revision=c374e410cf7db90933260cbd3a49be0e5af2fa85
ThreadRegistration changes: https://treeherder.mozilla.org/jobs?repo=try&revision=c26c7d10a79d7cb4dd51238f53b314d17095b62a&selectedTaskRun=IUlVrHO1TmKFkHIJbTsnkQ.0
ThreadRegistry changes: https://treeherder.mozilla.org/jobs?repo=try&revision=031d8a7d3dac1db63691feb581a2d45837db2af5&selectedTaskRun=Ispe3EQpTdqiZvc_g09j5w.0

As I noted in comment 2, I think the better code makes this small change worth it.
So at this point, I would say WONTFIX.

Andra, what do you think?

Flags: needinfo?(aesanu)

If the magnitude value is not concerning, since you have done some research, it should be okay.
The threshold for build_metrics is 100kb, the alert above has the value around 200kb.

Flags: needinfo?(aesanu)
Status: NEW → ASSIGNED
Depends on: 1736293

I may continue to look at ways to reduce the profiler code footprint, but I won't be tackling this particular one here, being a "regression" by bug 1722261, which I argued in comment 1 is really needed. And also, since it landed, it has already helped reducing crash reports massively, e.g. profiler_register_thread had hundred of crashes per version, there have been none in/after 93 after bug 1722261 landed.

I've filed bug 1736293 to look at any ways to reduce the code size.
But it's not directly related to the source of this size regression here, so I'll mark this bug WONTFIX.

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → WONTFIX
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.