Closed Bug 831507 Opened 7 years ago Closed 7 years ago

GC: Get Generational barriers in the Baseline JIT

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 6 obsolete files)

We need to generate code to update the store buffer whenever we update a gc pointer. This should look at sites that need incremental barriers and expend this to initializing writes (which incremental barriers don't care about). In JM this was the initprop and initelem opcodes, plus misc code in stubs.

The post barrier verifier will allow us to verify that we have caught all of the places that we need to update the store buffer.
Attached patch v0 (obsolete) — Splinter Review
When Ion marks the callee token, it updates the frame with the new value afterwards. Baseline needs to do the same thing.
Assignee: general → terrence
Status: NEW → ASSIGNED
Attachment #744737 - Flags: review?(kvijayan)
Attachment #744737 - Flags: review?(kvijayan) → review+
Attached patch wip0 (obsolete) — Splinter Review
This adds a post barrier to protect writes to the scope. It's my first baseline patch and I'm pretty sure it could be done better. We're going to need similar code in a couple other places, so any feedback you can give here would be helpful. With just 851057 + deps + this patch, we are down to 15 failures on jit-tests with --no-jm --baseline-eager.
Attachment #748325 - Flags: review?(kvijayan)
Comment on attachment 748325 [details] [diff] [review]
wip0

Jan pointed out that we should be able to use the same code as ionmonkey to implement barriers here. I'll ask if I have questions about the register situation.
Attachment #748325 - Flags: review?(kvijayan)
Attached patch wip2 (obsolete) — Splinter Review
Thanks for offering to work on this Kannan!
Attachment #748325 - Attachment is obsolete: true
Attached patch Fixed up patch. (obsolete) — Splinter Review
This passes all jit-tests with '--no-ion --no-jm --no-asmjs --baseline-eager'.

When I run jit-tests with 'gczeal(2)', there are about 6 errors that show up, but I tracked down the first two and they don't seem to be jit-related specifically.

Terrence: Have you tried running with 'gczeal(2)' and do you know if there are known failures with it?
Flags: needinfo?(terrence)
Clearing needinfo.
Flags: needinfo?(terrence)
Comment on attachment 753888 [details] [diff] [review]
Fixed up patch.

Sorry to load a hefty review on you Jan.  The changes touch a bunch of baseline code and I want someone with a good understanding of that code to take a look at this.

Changed the OOL post-barrier code for the mainline generated jitcode to use a callWithABI, but the post-barrier code in stubs is still using a VMCall.  There are a few optimizations left to do (guards on the callWithABI so that we don't invoke it for non-tenured objects, changing the stubcode implementation of the post-barrier to a callWithABI instead of a vm call), but I'll make those into delta patches.
Attachment #753888 - Flags: review?(jdemooij)
Comment on attachment 753888 [details] [diff] [review]
Fixed up patch.

Canceling review until further issues fixed.  Did a 32-bit build and it doesn't compile ;)
Attachment #753888 - Flags: review?(jdemooij)
Blocks: 706885
Blocks: 673454
No longer blocks: GenerationalGC
Attached patch Working patch (obsolete) — Splinter Review
This patch now passes tests on x86-32.  Two tests fail, but it doesn't seem related to the patch.

Will put it up for review after making sure ARM works ok.
Attachment #753888 - Attachment is obsolete: true
There are several errors on arm, but it seems to be an unrelated issue marty is looking into.  Putting this up for r? now.
Attachment #753977 - Flags: review?(jdemooij)
Attached patch Baseline GGC. (obsolete) — Splinter Review
Rebased to tip.
Attachment #744737 - Attachment is obsolete: true
Attachment #752843 - Attachment is obsolete: true
Attachment #753977 - Attachment is obsolete: true
Attachment #753977 - Flags: review?(jdemooij)
Attachment #754571 - Flags: review?(jdemooij)
Terrence:

Jan is reviewing the patch, and was wondering whether we need to do the explicit (Object,Slot) write to the store buffer from the baseline code, or whether we can get away with just writing the object alone like Ion does.

Some discussion was had on IRC and Brian indicated that in his testing it didn't make that much of a perf difference, even in pahtlogical cases, to write an object pointer to the store buffer instead of distinguishing it by slot.

Your thoughts?
Flags: needinfo?(terrence)
Ah, that sounds reasonable. My thinking was that since baseline compiles so much more, we'd be reasonably likely to hit pathological cases. If Brian says that doesn't actually matter much in practice, we should switch to using the whole-object barrier where it makes the code simpler.
Flags: needinfo?(terrence)
Attached patch Baseline GGC v2Splinter Review
Updated version of previous patch with following changes:

1. PostWriteBarrierHeapSlot has been removed, and all uses converted to just PostWriteBarrier instead.
2. The call to PostWriteBarrier from IC stubs is now a callWithABI instead of a VMCall.
3. In BaselineCompiler's code emission for JSOP_SETALIASEDVAR, inline guards checking the object pointer against the nursery space boundaries have been added.  The guards are emitted inline in the compiled code, instead of in the OOL code.
Attachment #754571 - Attachment is obsolete: true
Attachment #754571 - Flags: review?(jdemooij)
Attachment #755436 - Flags: review?(jdemooij)
Comment on attachment 755436 [details] [diff] [review]
Baseline GGC v2

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

Nice, glad you got this working so quickly without requiring major changes. r=me with comments below addressed.

::: js/src/ion/BaselineCompiler.cpp
@@ +274,5 @@
>  
> +#ifdef JSGC_GENERATIONAL
> +// On input:
> +//  R2.scratchReg() contains object being written to.
> +//  R1.scratchReg() contains slot index being written to.

Nit: this line can be removed now.

@@ +1792,5 @@
> +
> +    Nursery &nursery = cx->runtime->gcNursery;
> +    Label skipBarrier;
> +    Label isTenured;
> +    masm.branchPtr(Assembler::Below, objReg, ImmWord(nursery.start()), &isTenured);

Ion first does an is-value-an-object check, we should probably do that here too (avoids the call in many cases but also avoids unnecessarily adding the object to the store buffer).

::: js/src/ion/BaselineIC.cpp
@@ +646,5 @@
> +    Nursery &nursery = cx->runtime->gcNursery;
> +
> +    Label skipBarrier;
> +    Label isTenured;
> +    masm.branchPtr(Assembler::Below, obj, ImmWord(nursery.start()), &isTenured);

See the other comment about the is-object check for the value we're writing. It would be nice if we could do that check here, but else we can also do it in the callers.

@@ +653,5 @@
> +
> +    // void PostWriteBarrier(JSRuntime *rt, JSObject *obj);
> +    masm.PushRegsInMask(saveRegs);
> +#ifdef JS_CPU_ARM
> +    masm.push(BaselineTailCallReg);

To avoid saving non-volatile regs we can do something like:

saveRegs = GeneralRegisterSet::Union(saveRegs, GeneralRegisterSet::Volatile());

And nit: it's slightly simpler to do this at the start of the function:

#ifdef JS_CPU_ARM
saveRegs.add(BaselineTailCallReg);
#endif

@@ +4399,5 @@
> +    regs.add(key);
> +    regs.add(tmpVal);
> +#ifdef JSGC_GENERATIONAL
> +    {
> +        Register r = regs.takeAnyExcluding(BaselineTailCallReg);

On ARM the TailCallReg shouldn't be part of availableGeneralRegs() and on x86/x64 the return address is on the stack. It would be nice if we could use regs.getAny() in the call below (and similar calls in other stubs).

::: js/src/ion/BaselineIC.h
@@ +743,5 @@
>              return false;
>          }
>      }
>  
> +    static bool NeedsPostBarrier(ICStub::Kind kind) {

We can remove this function now.

@@ +4540,5 @@
>  
>    protected: // Protected to silence Clang warning.
>      HeapPtrTypeObject type_;
>      HeapPtrShape shape_;
> +    uint32_t slot_;

And this field (also in the Compiler class).

@@ +4610,5 @@
>  
>    protected: // Protected to silence Clang warning.
>      HeapPtrTypeObject type_;
>      HeapPtrShape newShape_;
> +    uint32_t slot_;

Ditto.

@@ +4685,5 @@
>      RootedObject obj_;
>      RootedShape oldShape_;
>      size_t protoChainDepth_;
>      bool isFixedSlot_;
> +    uint32_t slot_;

And here.
Attachment #755436 - Flags: review?(jdemooij) → review+
Subsequent push to fix 32-bit oranges: https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd1bae06bb7

Subsequent backout of both pushes because of dromaeo orange:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6858682eab1d
(The patch does also not address all review comments.)
Depends on: 877835
https://hg.mozilla.org/mozilla-central/rev/e7634d727b1a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.