1977367 - Consider nursery-allocating js::GetterSetter

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

In my local profiles of long ChatGPT chats (bug 1963578), I sometimes see a number of FULL_CELL_PTR_OBJ_BUFFER minor GCs. I looked into this and ChatGPT has some code that allocates a lot of JS objects with a getter property (we allocate about a million GetterSetter things for this). In this case the GetterSetter is tenured, which then results in always tenuring a number of other objects (the lambda getter function, its environment chain etc).

I wrote a prototype patch that nursery-allocates GetterSetter GC things and with that we still allocate the same number of them, but we tenure only a few percent of them for this ChatGPT chat, so it seems pretty effective.

The main thing missing from my hacky patch (other than cleanup) is that it doesn't support nursery GetterSetters for Ion compilation. This would need more work, probably something like MNurseryObject (this uses an index into a nursery-objects vector instead of a raw JSObject*).

Comment 1

[Jan de Mooij [:jandem]]

Attachment: WIP: Bug 1977367 - Hack: nursery-allocate GetterSetter.

Comment 2

[Jan de Mooij [:jandem]]

Here is a micro-benchmark that improves from 2676 ms to 1653 ms with this patch. I'm not sure how representative it is of what ChatGPT is doing.

function f() {
  var t = Date.now();
  for (var i = 0; i < 10_000_000; i++) {
    o = {arr: [{}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {}]};
    Object.defineProperty(o, "x", {get: function() { return o; }});
  }
  print(Date.now() - t);
}
f();

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1977367 part 1 - Change CacheIR WeakGetterSetterField to WeakValueField. r?jonco!

WeakGetterSetterField was only used for the GuardHasGetterSetter CacheIR instruction
and that instruction isn't very common. Changing this to a WeakValue makes it easier to
support nursery-allocated GetterSetters in Warp in later patches.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1977367 part 2 - Use HeapPtr for IonScript's Value list. r?jonco!

This now matches the nursery-objects list that's stored in IonScript.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1977367 part 3 - Minor cleanup for Ion nursery object code. r?jonco!

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1977367 part 4 - Support nursery Values in Warp. r?jonco!

This will make it possible to nursery-allocate GetterSetter while still being
able to transpile the CacheIR instructions for it to MIR. Unrelated to that,
supporting nursery values will also make it easier to bake in constant property
values based on fuses.

The approach is very similar to the nursery object support we already have but with some
small differences:

For objects we use WarpObjectField which uses low bit tagging to indicate it's
a nursery index instead of a JSObject*. That's more complicated for boxed Values
so this patch adds ValueOrNurseryValueIndex and it uses MagicUint32Value to
store the index.

The nursery object list is copied directly to the IonScript but because the
IonScript already has a list of Value constants, we can reuse that list for
the nursery Values.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1977367 part 5 - Change value of TraceKind::GetterSetter. r?jonco!

This is needed for the next patch because the nursery kinds need to come first.

In Value::traceKind we now need an extra check for symbols, but because
JSVAL_TYPE_SYMBOL and JSVAL_TYPE_PRIVATE_GCTHING are adjacent the compiler
will hopefully be able to optimize the common case with a single branch.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1977367 part 6 - Support allocating GetterSetter in the nursery. r?jonco!

The ChatGPT UI has code that sometimes allocates millions of GetterSetters
for a getter property that's a different function clone each time. This puts pressure
on the GC because we tenured these arrow functions and also other things they keep
alive through their environment chain.

With this patch we try to nursery-allocate GetterSetters for properties of objects
in the nursery. For ChatGPT we tenure a very small number of all GetterSetters we allocate
in the nursery so it's very effective for that workload. Speedometer and JetStream
seem unaffected.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5af8e26e558d
https://hg.mozilla.org/integration/autoland/rev/90fa5bf2b748
part 1 - Change CacheIR WeakGetterSetterField to WeakValueField. r=jonco
https://github.com/mozilla-firefox/firefox/commit/36ea0a2df12e
https://hg.mozilla.org/integration/autoland/rev/6b2308151f74
part 2 - Use HeapPtr for IonScript's Value list. r=jonco
https://github.com/mozilla-firefox/firefox/commit/7386b91f80ed
https://hg.mozilla.org/integration/autoland/rev/57cb2b778dca
part 3 - Minor cleanup for Ion nursery object code. r=jonco
https://github.com/mozilla-firefox/firefox/commit/e7b58e8d69af
https://hg.mozilla.org/integration/autoland/rev/c9c5042c45db
part 4 - Support nursery Values in Warp. r=jonco
https://github.com/mozilla-firefox/firefox/commit/903d7241c418
https://hg.mozilla.org/integration/autoland/rev/9eff9a1867fc
part 5 - Change value of TraceKind::GetterSetter. r=jonco
https://github.com/mozilla-firefox/firefox/commit/201a4a6bc640
https://hg.mozilla.org/integration/autoland/rev/36fb219b5e5f
part 6 - Support allocating GetterSetter in the nursery. r=jonco

Comment 10

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/90fa5bf2b748
https://hg.mozilla.org/mozilla-central/rev/6b2308151f74
https://hg.mozilla.org/mozilla-central/rev/57cb2b778dca
https://hg.mozilla.org/mozilla-central/rev/c9c5042c45db
https://hg.mozilla.org/mozilla-central/rev/9eff9a1867fc
https://hg.mozilla.org/mozilla-central/rev/36fb219b5e5f

Comment 11

[Mayank Bansal]

6.6% improvemetn on Ares6-basic-firstiteration

Comment 12

[Florin Bilt]

(In reply to Cristina Horotan [:chorotan] from comment #10)

https://hg.mozilla.org/mozilla-central/rev/90fa5bf2b748
https://hg.mozilla.org/mozilla-central/rev/6b2308151f74
https://hg.mozilla.org/mozilla-central/rev/57cb2b778dca
https://hg.mozilla.org/mozilla-central/rev/c9c5042c45db
https://hg.mozilla.org/mozilla-central/rev/9eff9a1867fc
https://hg.mozilla.org/mozilla-central/rev/36fb219b5e5f

Perfherder has detected a browsertime performance change from push 36fb219b5e5fe9c5ee71a0a519915c82610ae1cd.

If you have any questions, 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.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
4%	youtube fcp	linux1804-64-shippable-qr	fission warm webrender	180.65 -> 174.11	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.

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 46079

The following documentation link provides more information about this command.

Comment 13

[Acasandrei Beatrice (needinfo me)]

(In reply to Pulsebot from comment #9)

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5af8e26e558d
https://hg.mozilla.org/integration/autoland/rev/90fa5bf2b748
part 1 - Change CacheIR WeakGetterSetterField to WeakValueField. r=jonco
https://github.com/mozilla-firefox/firefox/commit/36ea0a2df12e
https://hg.mozilla.org/integration/autoland/rev/6b2308151f74
part 2 - Use HeapPtr for IonScript's Value list. r=jonco
https://github.com/mozilla-firefox/firefox/commit/7386b91f80ed
https://hg.mozilla.org/integration/autoland/rev/57cb2b778dca
part 3 - Minor cleanup for Ion nursery object code. r=jonco
https://github.com/mozilla-firefox/firefox/commit/e7b58e8d69af
https://hg.mozilla.org/integration/autoland/rev/c9c5042c45db
part 4 - Support nursery Values in Warp. r=jonco
https://github.com/mozilla-firefox/firefox/commit/903d7241c418
https://hg.mozilla.org/integration/autoland/rev/9eff9a1867fc
part 5 - Change value of TraceKind::GetterSetter. r=jonco
https://github.com/mozilla-firefox/firefox/commit/201a4a6bc640
https://hg.mozilla.org/integration/autoland/rev/36fb219b5e5f
part 6 - Support allocating GetterSetter in the nursery. r=jonco

Perfherder has detected a talos performance change from push 36fb219b5e5fe9c5ee71a0a519915c82610ae1cd.

If you have any questions, 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.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
7%	pdfpaint bug1826783.pdf	macosx1470-64-shippable	e10s fission stylo webrender-sw	554.23 -> 514.45

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 46074

The following documentation link provides more information about this command.