1983704 - Simplify AutoTArray guts

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Emilio Cobos Álvarez [:emilio]]

Landing this separate from bug 1978229 for convenience and easier tracking of potential regressions.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1983704 - Simplify AutoTArray guts. r=nika!

While looking into the previous patch, I realized there's a scary
mismatch between what the is-auto bit means for rust's ThinVec and
Gecko's AutoTArray.

Rust's ThinVec assumes that the is-auto bit means that the buffer itself
is stack-allocated, but that's not correct: Gecko will set mIsAutoArray
for heap-allocated buffers that replace a non-heap allocated buffer.
This is used so that when shrinking the array we can go back to point to
the stack-allocated buffer. This is all a bit hacky as noted by the
various comments here.

The point is that, if we were to get here with one such buffer, we'd
leak: https://searchfox.org/mozilla-central/rev/1177f9aa639e8bbb6633441064ab9dbc846a1998/third_party/rust/thin-vec/src/lib.rs#1673

I think right now we can't even get there, except if somehow you
mem::swap or so a ThinVec, which is rather dangerous anyways as
explained in the Crate's documentation.

I'm not convinced that's worth preserving all that complexity, as most
AutoTArrays are temporary. Simplify how AutoTArray is dealt with, so
that the rust usage is sound.

I made AutoTArray::Clear() point back to the stack-allocated buffer
because it's an easy way of getting the old behavior back if some
consumer needs it... I'll of course run this through performance
tests...

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1983704 - Stop passing alignment around to nsTArray_base. r=nika!

After the previous patch it's unused. Conceptually it's the right thing
to do if nsTArray wanted to ever deal with over-aligned types. But that
doesn't seem to be the case for now, so maybe worth taking the
simplification?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4f16a872b300
https://hg.mozilla.org/integration/autoland/rev/bd509f289247
Simplify AutoTArray guts. r=nika
https://github.com/mozilla-firefox/firefox/commit/05503dcf89f0
https://hg.mozilla.org/integration/autoland/rev/6cf15ac0d796
Stop passing alignment around to nsTArray_base. r=nika
https://github.com/mozilla-firefox/firefox/commit/360e944f1efd
https://hg.mozilla.org/integration/autoland/rev/1035a0a93512
apply code formatting via Lando

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2dfc8587c8c0
https://hg.mozilla.org/integration/autoland/rev/b38d5edaed3c
Revert "Bug 1983704: apply code formatting via Lando" for causing assertion failures.

Comment 5

[agoloman]

Backed out for causing assertion failures.

• Backout link
• Push with failures
• Failure Log

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a6e5180134a8
https://hg.mozilla.org/integration/autoland/rev/9b0e0cbbbc4f
Simplify AutoTArray guts. r=nika
https://github.com/mozilla-firefox/firefox/commit/3d57bb3cfd89
https://hg.mozilla.org/integration/autoland/rev/db835f2a3f57
Stop passing alignment around to nsTArray_base. r=nika

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/9b0e0cbbbc4f
https://hg.mozilla.org/mozilla-central/rev/db835f2a3f57

Comment 8

[Mayank Bansal]

Improved the size of the installer on windows-non-shippable by 247KB (0.2%)

Comment 9

[Mayank Bansal]

0.7% improvement on heap-unclassified
0.3% on Base content explicit

Comment 10

[Florin Bilt]

(In reply to Pulsebot from comment #4)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2dfc8587c8c0
https://hg.mozilla.org/integration/autoland/rev/b38d5edaed3c
Revert "Bug 1983704: apply code formatting via Lando" for causing assertion
failures.

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

Please acknowledge, and begin investigating this alert within 3 business days, or the patch(es) may be backed out in accordance with our regression policy. Our guide to handling regression bugs has information about how you can proceed with this investigation.

If you have any questions or need any help with the investigation, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
5%	speedometer Preact-TodoMVC/CompletingAllItems	macosx1500-aarch64-shippable	fission webrender	1.65 -> 1.73	Before/After
4%	speedometer VueJS-TodoMVC/CompletingAllItems	macosx1500-aarch64-shippable	fission webrender	2.38 -> 2.47	Before/After
3%	speedometer Inferno-TodoMVC/CompletingAllItems	macosx1500-aarch64-shippable	fission webrender	7.50 -> 7.76	Before/After
3%	speedometer Preact-TodoMVC	macosx1500-aarch64-shippable	fission webrender	5.05 -> 5.19	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
7%	speedometer VueJS-TodoMVC/Adding100Items/Sync	linux1804-64-shippable-qr	fission webrender	3.41 -> 3.15
7%	speedometer Preact-TodoMVC/Adding100Items/Sync	linux1804-64-shippable-qr	fission webrender	1.87 -> 1.73

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

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

You can run all of these tests on try with ./mach try perf --alert 46372

The following documentation link provides more information about this command.