1480524 - Convert NewObject IC to CacheIR

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Convert the NewObject code to use CacheIR

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Use CacheIR version of NewObject for Baseline

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Add Ion support to CacheIR NewObject stub

I'm open to some suggestions about better lifetime management for the template
object, especially if we can avoid modifying the IonIC trace code!

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Remove SharedStubs support for NewObject

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

The first patch also has a question about the allocation metadata guard; the shared IC uses a runtime guard, but my reading of the setter suggests that maybe we can get away without it so long as we don't attach an IC while one exists, as we will toss the JIT code when we set the guard.

Comment 5

[Jan de Mooij [:jandem]]

Compacting GC can move GC things so we risk subtle bugs if we bake in GC pointers without updating them (for example when a JSObject address is reused for a different object after a compacting GC we could unexpectedly reuse the stub code because the CacheIR will be identical).

One option is to store a counter (as uint64_t) in JitRuntime (something like [0]), increment that, and bake that in, to force different CacheIR for each stub. Another option is to special case by checking |kind| here [1].

[0] https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/js/src/jit/JitRealm.h#215

[1] https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/js/src/jit/BaselineCacheIRCompiler.cpp#2188-2189

Comment 6

[Jan de Mooij [:jandem]]

The IonBuilder fast path giving up if the template object has dynamic slots doesn't make sense to me:

https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/js/src/jit/IonBuilder.cpp#6268

If I remove that check, jit-tests still pass (except for recover-objects.js but that's caused by the test I think). Looking at the history of that code, Hannes added it but it's unclear why.

Would you mind checking if removing that condition helps performance without having the NewObject shared stub in Ion? The NewObject Baseline IC is special enough (the unshared code part) that I'd still prefer not having a CacheIR/Ion IC for this if possible.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

So I ran the experiment suggested (disable the Ion stubs, but also allow dynamic slotted template objects to hit the fast path in Ion). The results on perfherder are here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=778b9623c5ad&framework=1&showOnlyComparable=1&selectedTimeRange=172800 

In summary: No high confidence regressions, though "tp6_youtube opt 1_thread e10s stylo" in linux64-qr may show a 5% regression, but it still isn't clear after 20 runs. 

I'll update the patches on this bug to only have Baseline support (using stub fields to ensure correct GC behaviour, and a counter to disambiguate baseline stubs).

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 1] Use CacheIR version of NewObject for Baseline

This instead uses a counter to disambiguate caches, and uses the pre-existing
stub field machinery to hold the JSObject*.

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 2] Remove Ion NewObject stub, and remove limitation on template object dynamic slots for fast path

This ends up converting recover-object-bug1175233.js into a permafail (as you
suggested earlier). 

I'll have to investigate why tomorrow, but in the mean time I'm wondering if
we'll be able to land this in light of the performance testing in Comment 7.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 3] Move ICNewObject_Fallback out of SharedIC and remove non-CacheIR code generation

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

Whoops: I meant to say recover-objects.js ( recover-object-bug1175233.js is the intermittent classification it gets)

Comment 12

[Matthew Gaudet (he/him) [:mgaudet]]

The failing line is this one [1], asserting the object is not recovered on bailout. 

After enabling the fast-path code in Ion in Part 2 (Attachment #8998357 [details] [diff]) this appears to no longer be true. Can I just flip the sense of the assertion? 


https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/js/src/jit-test/tests/ion/recover-objects.js#163

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8998357 [details] [diff] [review]
[Part 2] Remove Ion NewObject stub, and remove limitation on template object dynamic slots for fast path

Review of attachment 8998357 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ We can keep an eye on Talos/AWFY but I think this is fine.

As for the test, I agree we can just change it from false => true. r=me on that too.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8998356 [details] [diff] [review]
[Part 1] Use CacheIR version of NewObject for Baseline

Review of attachment 8998356 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, r=me with comments addressed

::: js/src/jit/CacheIR.cpp
@@ +5450,5 @@
> +
> +bool
> +NewObjectIRGenerator::tryAttachStub()
> +{
> +    if (!templateObject_) {

With the other comment addressed, we can remove this check.

@@ +5470,5 @@
> +        trackAttached(IRGenerator::NotAttached);
> +        return false;
> +    }
> +    // Stub doesn't support metadata builder
> +    // MG:XXX: But it does guard, so technically the stub would

I think it makes sense to remove this XXX comment and keep it like this.

@@ +5479,5 @@
> +    //         be able to get away without the runtime guard,
> +    //         so long as we don't attach stubs when the metadata
> +    //         builder is attached.
> +    if (cx_->realm()->hasAllocationMetadataBuilder())
> +        return false;

Should there be a trackAttached(IRGenerator::NotAttached); call here?

::: js/src/jit/CacheIR.h
@@ +1256,5 @@
> +        addStubField(uintptr_t(templateObj), StubField::Type::JSObject);
> +        // Bake in a monotonically increasing number to ensure we differentiate
> +        // between different baseline stubs that otherwise might share
> +        // stub code.
> +        writeUint32Immediate(cx_->runtime()->jitRuntime()->nextDisambiguationId());

We should write the whole uint64_t. There's no writeUint64Immediate, one option is to do something like:

uint64_t id = ..;
writeUint32Immediate(id & UINT32_MAX);
writeUint32Immediate(id >> 32);

::: js/src/jit/CacheIRCompiler.h
@@ -710,5 @@
>      void emitLoadStubField(StubFieldOffset val, Register dest);
>      void emitLoadStubFieldConstant(StubFieldOffset val, Register dest);
>  
>      uintptr_t readStubWord(uint32_t offset, StubField::Type type) {
> -        MOZ_ASSERT(stubFieldPolicy_ == StubFieldPolicy::Constant);

I'd prefer keeping this assertion and instead inlining the code below in objectStubFieldUnchecked.

It's still a bit confusing to use the "ForIon" name because this is Baseline, but it's also not the end of the world - it is a very unusual case for Baseline anyway.

::: js/src/jit/JitRealm.h
@@ +333,5 @@
>      void ionLazyLinkListRemove(JSRuntime* rt, js::jit::IonBuilder* builder);
>      void ionLazyLinkListAdd(JSRuntime* rt, js::jit::IonBuilder* builder);
> +
> +    uint32_t nextDisambiguationId() {
> +        return disambiguationId_++; 

Nit: remove trailing whitespace

::: js/src/jit/SharedIC.cpp
@@ +1829,5 @@
>  
>          if (obj && !obj->isSingleton() &&
>              !obj->group()->maybePreliminaryObjectsDontCheckGeneration())
>          {
> +            templateObject = NewObjectOperation(cx, script, pc, TenuredObject);

Nit: convention is to OOM-check immediately after the call instead of in the else-branch below:

if (!templateObject)
    return false;

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: [Part 3] Move ICNewObject_Fallback out of SharedIC and remove non-CacheIR code generation

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8999746 [details] [diff] [review]
[Part 3] Move ICNewObject_Fallback out of SharedIC and remove non-CacheIR code generation

Review of attachment 8999746 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineIC.cpp
@@ +5200,5 @@
> +
> +    RootedObject obj(cx);
> +
> +    RootedObject templateObject(cx, stub->templateObject());
> +    if (templateObject) {

So we only attach an optimized stub if there's no template object yet? What if GC discards optimized stubs, I think we will then keep the template object and never attach an IC stub again?

If this matches the old behavior, would you mind filing a follow-up bug?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a09332be9ac
[Part 1] Use CacheIR version of NewObject for Baseline r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab47d3f47325
[Part 2] Remove Ion NewObject stub, and remove limitation on template object dynamic slots for fast path r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e79db1588c2
[Part 3] Move ICNewObject_Fallback out of SharedIC and remove non-CacheIR code generation r=jandem

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/2a09332be9ac
https://hg.mozilla.org/mozilla-central/rev/ab47d3f47325
https://hg.mozilla.org/mozilla-central/rev/2e79db1588c2