851057 - IonMonkey: Integrate with bump allocation nursery

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: WIP

Bug 706885 adds a bump allocation nursery that works with the interpreter.  JIT integration is needed as well.  The attached patch is a WIP for this.  The main fixes are allocating new objects out of the nursery and adding post write barriers when adding tenured -> nursery edges between objects.  This has only barely been tuned for a couple simple examples and in particular the write barriers are as bare bones as possible right now.  Also adds minimal JIT integration with JM --- JM will just make a stub call when allocating objects or writing an object to the heap.  Fails about 20 jit-tests with --ion-eager --ggc.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: WIP

New WIP, still fails a few jit-tests --ion-eager --ggc but only for things I think are unrelated to the JIT (weak maps, proxies, ...).  Still crashing occasionally on v8bench but usually works; this also adds fixes and optimizations to the object buffer and related code so that we never fall back to a full mark on v8bench.  Typical scores I get on v8bench (x86/darwin):

trunk (rather, rev 7b3da1dae19a, at the point the ggc stuff was forked of):

Richards: 11830
DeltaBlue: 12960
Crypto: 12551
RayTrace: 12210
EarleyBoyer: 13136
RegExp: 1907
Splay: 9290
NavierStokes: 17554
----
Score (version 7): 9949

ggc:

Richards: 10926
DeltaBlue: 15432
Crypto: 12129
RayTrace: 29422
EarleyBoyer: 9907
RegExp: 1725
Splay: 2304
NavierStokes: 17554
----
Score (version 7): 8961

Notes:

- Splay took a massive hit, as expected.  Will be fixed after the initial landing by detecting that splay tree nodes and associated data are all being tenured and allocating them directly in the major heap.  We can do this by figuring out the objects' allocation sites by looking at their type objects.

- Earley-boyer took a big hit rather than the expected big improvement.  Main thing that needs investigating.

- Deltablue got a nice improvement.  Still a little slower than d8, hopefully this one will improve more with tuning or with whatever fixes the earley-boyer issue.

- Raytrace got a huge improvement.  Part of this is that we aren't doing major GCs and throwing away jitcode now, but even if I add a gcPreserveCode() trunk only improves to 17k.

- Other tests don't seem to be affected that much, as expected.

Comment 2

[Alfred Kayser]

I think this is for all platforms?

Comment 3

[Brian Hackett [Laid off!]]

Attachment: WIP

Last WIP until bug 706885 lands.  This adds various tweaks, not much related to the JIT itself but addressing various performance concerns.

- Always trace globals in MarkRuntime, and don't add post barriers for writes to globals.  Tons of store buffer entries were being added for global writes in e-b.  These could be consolidated but it seems better to avoid them entirely.  This needs a mechanism to avoid tracing hundreds of globals in a browser setting but is I think a nice mechanism to have in the general case.

- Allow the nursery to grow up to 8MB, dynamically resizing it according to how many objects are being promoted (higher promotion rate ==> larger nursery).  e-b wants a big nursery, v8 also grows to 8MB on it.

- Don't use generic tracing code for marking during a minor collection, hand optimizing this to avoid overhead improves e-b by about 2000 points.

I now get scores with ggc like:

Richards: 11074
DeltaBlue: 14235
Crypto: 12926
RayTrace: 29792
EarleyBoyer: 14016
RegExp: 1717
Splay: 3315
NavierStokes: 17554
----
Score (version 7): 9799

If I transplant in a typical non-ggc score for splay (which we should be able to get to) then the overall score improves to 11146, a 12% improvement over trunk.  There is still plenty more perf improvement outside splay to do of course, though that can wait for bug 706885 and the baseline compiler.  I think in particular that JM stubbing all operations that can invoke a post barrier hurts these scores significantly.

Comment 5

[Terrence Cole [:terrence]]

Attachment: wip1

This is everything in WIP 1 except for (1) what I have already split out into the new blocking bugs, (2) dynamic nursery growth, which I will work on next, and (3) the methodjit bits. I think we should just drop support for JM and focus on getting baseline supported now.

There are still a couple FIXME's that need addressing and about 0.5% of the jit-tests are failing. The failures I've investigated so far are due to incorrect barriers on the constant pools and should be easy to address.

Comment 6

[Cameron Kaiser [:spectre]]

Re 3): I assume that merely means, for the moment, that methodjit won't work with GGC, as previously warned?

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 739820 [details] [diff] [review]
wip1

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

Looks okay to me!

::: js/src/ion/CodeGenerator.cpp
@@ +1114,5 @@
> +    if (!addOutOfLineCode(ool))
> +        return false;
> +
> +    ValueOperand value = ToValue(lir, LPostWriteBarrierV::Input);
> +    masm.branchTestObject(Assembler::NotEqual, value, ool->rejoin());

Are strings not in the nursery?

::: js/src/ion/IonMacroAssembler.cpp
@@ +442,5 @@
> +        loadPtr(AbsoluteAddress(nursery.addressOfPosition()), result);
> +        addPtr(Imm32(thingSize), result);
> +        branchPtr(Assembler::BelowOrEqual, AbsoluteAddress(nursery.addressOfCurrentEnd()), result, fail);
> +        storePtr(result, AbsoluteAddress(nursery.addressOfPosition()));
> +        subPtr(Imm32(thingSize), result);

Cool.

::: js/src/ion/LinearScan.cpp
@@ +555,5 @@
>                      // add a torn entry.
>                      if (!safepoint->addNunboxParts(*typeAlloc, *payloadAlloc))
>                          return false;
> +
> +                    if (payloadAlloc->isGeneralReg() && isSpilledAt(payloadInterval, inputOf(ins))) {

It's been a long time, so it would be good to have a detailed comment on why this is needed. Is it that... the payload has a canonical spill, but we're only recording the register entry if it exists?

Comment 8

[Terrence Cole [:terrence]]

(In reply to David Anderson [:dvander] from comment #7)
> Review of attachment 739820 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks okay to me!
> 
> ::: js/src/ion/CodeGenerator.cpp
> @@ +1114,5 @@
> > +    if (!addOutOfLineCode(ool))
> > +        return false;
> > +
> > +    ValueOperand value = ToValue(lir, LPostWriteBarrierV::Input);
> > +    masm.branchTestObject(Assembler::NotEqual, value, ool->rejoin());
> 
> Are strings not in the nursery?

Not yet: we decided to focus on objects first. String have many places that require the arena header and many places that just steal an internal pointer; we need to deal with these first.

> ::: js/src/ion/LinearScan.cpp
> @@ +555,5 @@
> >                      // add a torn entry.
> >                      if (!safepoint->addNunboxParts(*typeAlloc, *payloadAlloc))
> >                          return false;
> > +
> > +                    if (payloadAlloc->isGeneralReg() && isSpilledAt(payloadInterval, inputOf(ins))) {
> 
> It's been a long time, so it would be good to have a detailed comment on why
> this is needed. Is it that... the payload has a canonical spill, but we're
> only recording the register entry if it exists?

Brian?

Comment 9

[Brian Hackett [Laid off!]]

(In reply to David Anderson [:dvander] from comment #7)
> It's been a long time, so it would be good to have a detailed comment on why
> this is needed. Is it that... the payload has a canonical spill, but we're
> only recording the register entry if it exists?

For a given safepoint, LSRA can put the payload for a vreg in up to two places --- the canonical spill location, if it exists, and some register or other location for the vreg's interval covering that location.  Both are live, and both need to be recorded in the safepoint.  With a non-moving GC only one of the payload locations needs to be recorded.

Comment 10

[Terrence Cole [:terrence]]

Attachment: v0

Current status:
jittests --no-jm --no-baseline:               0 failures / 1 timeouts
jittests --no-jm --no-baseline --ion-eager:   6 failures / 0 timeouts
    (parallelarray/scatter[8-13].js except 9)
jstests --no-jm --no-baseline:                2 failures / 4 timeouts
    js1_8_5/regress/regress-595365-2.js
    js1_8_5/extensions/reflect-parse.js
jstests --no-jm --no-baseline --ion-eager:    4 failures / 6 timeouts
    js1_8_5/regress/regress-595365-2.js
    js1_8_5/extensions/sps-generators.js
    js1_8_5/extensions/typedarray.js
    js1_8_5/extensions/reflect-parse.js

I think this is ready for more eyes.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 748324 [details] [diff] [review]
v0

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

::: js/src/ion/LinearScan.cpp
@@ +556,5 @@
>                      if (!safepoint->addNunboxParts(*typeAlloc, *payloadAlloc))
>                          return false;
> +
> +                    if (payloadAlloc->isGeneralReg() && isSpilledAt(payloadInterval, inputOf(ins))) {
> +                        if (!safepoint->addNunboxParts(*typeAlloc, *payload->canonicalSpill()))

nit: please add the comment explaining this

Comment 12

[Terrence Cole [:terrence]]

Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=17dcf510b7bb

https://hg.mozilla.org/integration/mozilla-inbound/rev/de1c0246854c

Comment 13

[Phil Ringnalda (:philor)]

Reluctantly backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b54ce66659aa - one of the four things in that push, despite having been green on try, picked up causing failures in Android 2.2 reftest-1, reftest-2, and reftest-4 by the time it landed. Since I always blame the need to clobber when it's Android, I clobbered and retriggered on your push, and they still failed.

Comment 14

[Terrence Cole [:terrence]]

Yeah, incomprehensible is the right word. Naturally, all of those tests are green on my Try push as well.

Comment 15

[Phil Ringnalda (:philor)]

Yeah, just reland them all again, at a time when you'll be around to argue down anyone who wants to back you out - it cropped up again, someone else got backed out for something else plus this same thing, then it cropped up again after that backout.

Comment 16

[Terrence Cole [:terrence]]

Thanks, that's excellent to know! If inserting empty nodes into the MIR graph did actually cause the unrelated code in the image decoder library to stop working, I'd be seriously worried. On the other hand, that implies the image decoder library stops working at random for no reason whatsoever; not exactly comforting.

Comment 17

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/361fef4fb47a

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/361fef4fb47a