1934856 - Use buffer allocator for JSObject slots/elements

Status: RESOLVED FIXED

(Core :: JavaScript: GC, task, P2)

Description

[Jon Coppeard (:jonco)]

This bug is for the changes to use the memory allocator added in bug 1911537 for JSObject slots and elements.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1934856 - Part 1: Use buffer allocator for JSObject slots r?sfink,jandem

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1934856 - Part 2: Use buffer allocator for JSObject elements r?sfink,jandem

Comment 3

[Jon Coppeard (:jonco)]

Heads up - this may cause performance changes.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04fe040fd810
Part 1: Use buffer allocator for JSObject slots r=sfink,jandem
https://hg.mozilla.org/integration/autoland/rev/4b68a93c9b6b
Part 2: Use buffer allocator for JSObject elements r=sfink,jandem
https://hg.mozilla.org/integration/autoland/rev/9be74fa11145
apply code formatting via Lando

Comment 5

[Cristina Horotan [:chorotan]]

Backed out for causing valgring failures

Backout link

Push with failures

Failure log

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1934856 - Part 0: Don't recommit pages that contain previous allocations r?sfink

Currently BufferAllocator::recommitRegion recommits memory from the start of
the free region rouded down to the page size, but this may extend before the
start of the region and contain other live allocations. This is not a problem
except that recommit marks the memory as undefined and tools like valgrind
check can there flag valid accesses to these allocations as accessing undefined
memory.

We don't decommit the page containing the start of a free region in this case
so we just need to update what gets recommitted.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5d3e5da3f01
Part 0: Don't recommit pages that contain previous allocations r=sfink
https://hg.mozilla.org/integration/autoland/rev/0f1401774d50
Part 1: Use buffer allocator for JSObject slots r=sfink,jandem
https://hg.mozilla.org/integration/autoland/rev/d95fadc5b8ff
Part 2: Use buffer allocator for JSObject elements r=sfink,jandem

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/f5d3e5da3f01
https://hg.mozilla.org/mozilla-central/rev/0f1401774d50
https://hg.mozilla.org/mozilla-central/rev/d95fadc5b8ff

Comment 9

[Mayank Bansal]

Improvement

Kraken
3.1% improvement overall

• Driven by
• 10.5% improvement on audio-beat-detection
• 7.3% on audio-fft

less than 8% improvement on sunspider-string-unpack-code
less than 4.4% improvement on Ares6-Babylon-average-worst-case
18.5% on six-speed-map-set-object-es5
12.3% on web-tooling-benchmark-coffeescript

Jetstream2

• 12.7% on coffeescript-wtb
• 6.2% on pdfjs
• 6% on typescript-average

Regression

20% on Ares6-basicAverageWorstCase
Some regression on Ares6-ML-SteadyState
6.5% on Jetstream2-crypto-sha1-SP
50% on six-speed-map-set-es5

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Mayank Bansal from comment #9)
Good to see these improvements.

As for regressions, the Ares6 tests a very susceptible to GC timing. Changes in timing can cause 'worst case' times to regress even if the amount of time spent in GC does not change, because a single GC running during a test will affect the worst case time.

Comment 11

[Mayank Bansal]

This lead to an increase in Base Content JS memory numbers, but there is the corresponding decrease in heap-unclassified. This is good as it means more memory is being accounted/reported.
Grpahs: https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&replicates=0&series=autoland,4663315,1,4&series=autoland,4663317,1,4&timerange=1209600

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Mayank Bansal from comment #11)
The Base Content JS increase is because this now includes allocated but unused memory for slots/elements (i.e. empty space in allocated arenas). Previously jemalloc was used for this, but allocated but unused jemalloc memory is not included in this value.

Bug 1940066 should reduce this a bit.

I haven't investigated where the improvement in heap-unclassified came from.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Perfherder has detected a devtools performance change from push d95fadc5b8ff50c770be155a699c1c0aa8050925.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
9%	damp console.log-in-loop-content-process-typedarray	windows11-64-shippable-qr	e10s fission stylo webrender	62.09 -> 67.77

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
21%	damp console.log-in-loop-content-process-promise	windows11-64-shippable-qr	e10s fission stylo webrender	59.81 -> 47.08
12%	damp custom.netmonitor.close.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender	21.73 -> 19.14
12%	damp console.log-in-loop-content-process-node	windows11-64-shippable-qr	e10s fission stylo webrender	96.13 -> 84.81
10%	damp jstracer.server-performance.DAMP	windows11-64-shippable-qr	e10s fission stylo webrender	1,987.13 -> 1,793.33
6%	damp console.log-in-loop-content-process-nodelist	linux1804-64-shippable-qr	e10s fission stylo webrender	1,179.21 -> 1,108.14
...	...	...	...	...
2%	damp custom.jsdebugger.stepInNewSource.DAMP	windows11-64-shippable-qr	e10s fission stylo webrender	1,008.87 -> 985.22

As author of one of the patches included in that push, we need your help to address this regression.
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 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.

You can run these tests on try with ./mach try perf --alert 43297

For more information on performance sheriffing please see our FAQ.

Comment 14

[Jon Coppeard (:jonco)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
Since there are many improvements in the alert, we'll take the single regression.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jon Coppeard (:jonco) from comment #14)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
Since there are many improvements in the alert, we'll take the single regression.

I wouldn't look at the regression, it's probably some side-effect of the shifts in the other tests (especially since we're seeing it only on windows)