839258 - BaselineCompiler: Implement TypeUpdate and TypeMonitor stubs that handle combinations of basic types - space optimization.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Currently, our TypeUpdate and TypeMonitor IC chains will become somewhat expensive - attaching a new stub for every type that's seen.

At least for primitive types (Null, Undefined, Int, Double, etc.), we can consolidate these and generate an optimized stub for every subset of the primitive types.

Then, when updating TypeMonitor or TypeUpdate IC chains, with new primitive types, we can change the stub kind and code instead of adding a new stub.

E.g.:

Chain:  * ---> {Fallback}
Add Type Int32
Chain:  * ---> {Int32} ---> {Fallback}
Add Type Double
Chain:  * ---> {Int32|Double} ---> Fallback
Add TypeObject t1
Chain:  * ---> {Int32|Double} ---> {TypeObject:t1} ---> {Fallback}
Add Type Null
Chain:  * ---> {Int32|Double|Null} ---> {TypeObject:t1} ---> {Fallback}

This should save a bunch of space (and be a minor speed boost as well for highly polymorphic monitor and update chains).

If we handle the full set of possible subsets of:
{Double,Int32,Undefined,Boolean,Magic,String,Null,Object}, we're talking about 256 stub kinds.  We could reduce that by partitioning it into:

{Undefined|Int32|Double|Boolean} + {Undefined|Null|String|Object} + {Magic}

Which reduces it to 16 + 16 + 1 = 33 stub kinds.

Comment 1

[Brian Hackett [Laid off!]]

Can stubs just be stored in the atoms compartment, as is done for VM call and other Ion trampolines?  That seems like it would wipe out any concern about excessive stubs, as there would only be a single set of stubs shared throughout the runtime.

Comment 2

[Kannan Vijayan [:djvj]]

We talked about doing this before we started, but for some reason we ended up not doing that.

I think it was because we wanted to be able to collect stubcodes on GC, and sharing them made that hard (I suppose we could use refcounts to track that though.. but it complicates the design since IonCodes aren't refcounted).

Also, there are a few places where we bake in some compartment-specific pointers to the stubcode, but that can be fixed up.

We could also just adopt a partial approach where we globalize the more commonly used stubcodes (i.e. all the TypeUpdate/Monitor ones, maybe Integer/Double binary ops, getprop-native, etc.).

Comment 3

[Jan de Mooij [:jandem]]

Write barriers are enabled per-compartment, so we can't share stubs with write barriers across compartments (unless we always enable the barriers instead of patching them, but I'd like to avoid that).

Only a few stubs use write barriers though, the other ones we should store in the IonRuntime / atoms compartment.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Kannan Vijayan [:djvj] from comment #2)
> I think it was because we wanted to be able to collect stubcodes on GC, and
> sharing them made that hard

Yeah, it would be good to measure how much memory stubs use in a typical browsing session if/when we do this.

Comment 5

[Kannan Vijayan [:djvj]]

Just had a thought: amalgamation can be done for TypeObjects (and singletons) as well.

Currently, Type[Update|Monitor]_TypeObject records a single type object, and it uses 4 machine words for that.  Every additional TypeObject or singleton will incur the 4-word cost.

If we have these stubs keep a fixed-length array of up to 5 TypeObjects, then for the price of doubling the size of the TypeObject stub, we reduce the per-TypeObject overhead to 60% instead of 300%.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: Patch

I thought this was a problem under Box2d, but it wasn't.  But I have the patch written up anyway.  Might as well get it in.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 719122 [details] [diff] [review]
Patch

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

Sorry, I forgot to look at this yesterday. Looks great.

::: js/src/ion/BaselineIC.cpp
@@ +836,5 @@
> +        IonSpew(IonSpew_BaselineIC, "  %s TypeMonitor stub %p for primitive type %d",
> +                existingStub ? "Modified existing" : "Created new", stub, type);
> +
> +        if (!existingStub)
> +            addOptimizedMonitorStub(stub);

Nit: add a JS_ASSERT(!hasStub(TypeMonitor_PrimitiveSet)) to addOptimizedMonitorStub and addOptimizedUpdateStub, just in case.

@@ +979,5 @@
> +
> +    if (flags_ & TypeToFlag(JSVAL_TYPE_STRING))
> +        masm.branchTestString(Assembler::Equal, R0, &success);
> +
> +    if (flags_ & TypeToFlag(JSVAL_TYPE_OBJECT))

Nit: we should never see an object here, so you can turn this into an assert.

@@ +1181,5 @@
> +
> +    if (flags_ & TypeToFlag(JSVAL_TYPE_STRING))
> +        masm.branchTestString(Assembler::Equal, R0, &success);
> +
> +    if (flags_ & TypeToFlag(JSVAL_TYPE_OBJECT))

Same here.

Comment 8

[Kannan Vijayan [:djvj]]

> Nit: we should never see an object here, so you can turn this into an assert.

Did as you suggested, but I do expect that we'll be needing that bit later on, so I commented out the existing code with a descriptive comment above it.

I'm noticing several places in Octane where we add a bunch of object stubs and then max out the chain.  Ideally, once we can know for sure that the underlying typeset has generalized to any-object and discarded its TypeObject list, we should eliminate the individual typeobject and singleton stubs from the IC chain and set this flag on the primitive type set.

But that will require some extra changes to the type check IC chains - namely either increasing their maximum stub count, or adding "fat" stubs that check multiple TypeObjects or Singletons.  That's because currently, we need to wait until we get at least 256 of these before we can assume that the underlying typeset has generalized to any-object.

Comment 9

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/projects/ionmonkey/rev/ce761cd6345a

Comment 10

[Kannan Vijayan [:djvj]]

Bug: Need to put a write barrier before updating the stubs' code pointers.

Comment 11

[Kannan Vijayan [:djvj]]

Attachment: Patch to fix write barrier issue.

This should handle the write barrier related oranges showing up on TBPL due to the above push.

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 720029 [details] [diff] [review]
Patch to fix write barrier issue.

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

::: js/src/ion/BaselineIC.cpp
@@ +98,5 @@
> +    IonCode::writeBarrierPre(ionCode());
> +#endif
> +    stubCode_ = code->raw();
> +#ifdef JSGC_GENERATIONAL
> +    IonCode::writeBarrierPost(code, IonCode::ExecutableBackAddr(stubCode_));

Per IRC discussion this call should be removed.

Comment 13

[Kannan Vijayan [:djvj]]

Done and pushed: https://hg.mozilla.org/projects/ionmonkey/rev/40ae34ac43a8

Comment 14

[Kannan Vijayan [:djvj]]

Looking at the tbpl page for the clean version of these, there seems to be XPCshell orange introduced by this patch:
https://tbpl.mozilla.org/?tree=Ionmonkey&rev=f3306a47ed30

However, I backed out the two patches (original patch and write barrier fix) and pushed to try, which shows the same XPCshell failures:
https://tbpl.mozilla.org/?tree=Try&rev=fdc7aaf56b86

So now I'm confused, because I don't see the X orange in inbound.  I tried to look at other try builds, but they're all either full busts, or specific try builds that don't include X.

I'm not sure what's making the Xs show up all of a sudden and why they're not going away after reverting the patch(es) that seemed to have cause them.  Is this some issue with the try builders?

Comment 15

[Jan de Mooij [:jandem]]

The XPC orange came in with ce761cd6345a, so shouldn't that be the one you want to back out?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

The XPC failures are unrelated to your push. See below:
https://hg.mozilla.org/mozilla-central/rev/98cb2dc01041

Comment 17

[Kannan Vijayan [:djvj]]

> Jan:
> The XPC orange came in with ce761cd6345a, so shouldn't that be the one you
> want to back out?

Jan: I backed out the two pushes I made on top of the main push for this bug so that I could get a clean TBPL run of just the patch + the write barrier fix.  Also, those two patches were introducing other oranges too (see bug 846973).  If you can gimme a quick r? on that I can get these out the door today or tomorrow.  I'd like to start monday with these stubs out of the way.

I ran the try with an additional backout of the two pushes for this bug, that I saved on my patchqueue, just to see if the X orange was showing up there too.

> RyanVM:
> The XPC failures are unrelated to your push. See below:
> https://hg.mozilla.org/mozilla-central/rev/98cb2dc01041

Ryan: Thanks for letting me know.  I was getting concerned because I could not replicate the issue locally on my build.