Closed Bug 1727877 Opened 3 months ago Closed 3 months ago

3.2 - 3.17% Base Content Heap Unclassified / Base Content Heap Unclassified (Linux) regression on Wed August 25 2021

Categories

(Core :: Gecko Profiler, defect, P1)

defect

Tracking

()

RESOLVED FIXED
93 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox91 --- unaffected
firefox92 --- unaffected
firefox93 --- fixed

People

(Reporter: beatrice.acasandrei, Assigned: gerald)

References

(Regression)

Details

(Keywords: perf, perf-alert, regression)

Attachments

(1 file)

Perfherder has detected a awsy performance regression from push c8933e49a64bb79e25d333e7c832880048cb5712. 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)
3% Base Content Heap Unclassified linux1804-64-shippable-qr fission 2,062,059.83 -> 2,127,969.67
3% Base Content Heap Unclassified linux1804-64-shippable-qr fission 2,062,438.67 -> 2,127,772.67

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)

Thank you for the report.
My guess is that this is due to this patch, which is allocating a buffer in each thread that has a JSContext (i.e., it's running Javascript).
This helps with lowering the overhead of profiling, by not having to share a single buffer when working with mutliple Javascript runners (main thread and workers).

It should be possible to only allocate this buffer when the user is actually profiling; I didn't do it from the start, because it added complexity, and may reintroduce some unwanted locking... I will investigate next week.

Assignee: nobody → gsquelart
Flags: needinfo?(gsquelart)
Priority: -- → P1
Blocks: 1727754
Blocks: 1728056

The buffer was previously allocated as soon as a JSContext was present, meaning that some memory was used even when the profiler was not running, which is the case most of the time for most users.
This saves some memory, at the cost of having to lock the per-thread ThreadRegistration data when sampling a thread with a JSContext. This should have low contention risk, because it's mostly used when sampling on the thread, while the periodic sampler uses its own buffer so it doesn't need to lock the per-thread data.

Some results: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=281364c43774d44fbd33d953e4e988f9299afdd1&newProject=try&newRevision=eaf8449611a9bd7c3b04ee52d9f3629d908d83e2&framework=4&page=1&filter=base
E.g.:
Base Content Heap Unclassified opt fission: 2,127,347 -> 2,061,988 -3.07%

So it confirms that the always-allocated JS frame buffer was the issue, the attached patch reverses that; now the buffer is only allocated when profiling actually starts.

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6242f4c09c5
Only allocate JS frame buffer when thread is being profiled - r=canaltinova
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch

== Change summary for alert #31132 (as of Fri, 03 Sep 2021 04:51:05 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
3% Base Content Heap Unclassified linux1804-64-shippable-qr fission 2,125,948.33 -> 2,059,764.33
3% Base Content Heap Unclassified linux1804-64-shippable-qr fission 2,125,094.83 -> 2,059,624.67

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

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