657412 - TI: reviews for VM changes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

For TI to be able to land to TM, the changes it makes to the VM need to be reviewed.  Inference itself and related analyses should also get reviewed, though I don't know the time frame, and I don't know yet about inference-enabled changes to JM (will definitely need review when/if JM+TI is default-enabled, that ties into progress on IonMonkey).

It would be good to be able to land TI early in the next release cycle, around June 1, so I'd really like these reviews to be done by then.  Landing also depends on a stable TI-enabled tinderbox and little perf change with TI disabled.

Will clean up the diff, chop it up and attach 'patches' here for review (nothing will build if they are applied individually).

Comment 1

[Brian Hackett [Laid off!]]

Attachment: stack layout changes

Stack frame changes, mostly for frame inlining / expansion but also storage for info used when rejoining via the interpoline.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: script related changes

Changes to JSScript and bytecode, including type sets for property fetch opcodes, SETHOLE, per-script empty call shapes and extra script fields for type information.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: object changes

Changes to JSObject, including rearranged slot layout (bug 648321), changing out .proto for .type and .emptyShapes for .newType.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: array changes

Array changes, mainly managing packed-ness but also barriers for setting dense elements and length overflowing an uint32.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: type hooks and API changes

Changes to internal and API methods to take type information, and hooks where type information is updated dynamically due to barriers in object methods and unexpected type results from natives.  (dynamic results in the interpreter, e.g. integer overflows, are covered in the scripts patch).

Comment 6

[Brian Hackett [Laid off!]]

Attachment: XPCOM/DOM changes

Changes to XPCOM and the rest of the browser, mainly allowing us to get accurate types for Window objects and adding an option for turning inference on in the browser.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: miscellaneous

Miscellaneous changes.  Mostly tests, also (reviewed) GET*PROP removal and minor get-this-to-compile changes.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: analysis and inference guts

Guts of the inference algorithm and other bytecode analyses, also analysis memory management stuff.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: JaegerMonkey changes

Changes to JM.  Mainly new optimizations gated on inference being enabled, but a fair amount of refactoring and the redone recompilation mechanism (interpoline) are in play when inference is disabled.

Comment 10

[David Mandelin [:dmandelin]]

Comment on attachment 532857 [details] [diff] [review]
type hooks and API changes

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

I have a batch of questions.

::: js/src/jsapi.h
@@ +2180,5 @@
>  JS_SetPrototype(JSContext *cx, JSObject *obj, JSObject *proto);
>  
> +extern JS_PUBLIC_API(void)
> +JS_SplicePrototype(JSContext *cx, JSObject *obj, JSObject *proto);
> +

How is this API used? Does it have to be public? It seems like an obscure usage for JSAPI users.

@@ +2702,5 @@
> +JS_DefineUCFunctionWithType(JSContext *cx, JSObject *obj,
> +                            const jschar *name, size_t namelen, JSNative call,
> +                            uintN nargs, uintN attrs,
> +                            JSTypeHandler handler);
> +

In general, on the new *WithType APIs, how are they going to be used? Will the browser use them? Will JSAPI users use them? It seems basically appropriate to make these public APIs, but I'd like to know more about how they work. (Of course, they will also need documentation in MDC.)

::: js/src/jscntxt.h
@@ +1362,5 @@
> +
> +    /* Make a type object whose name is that of base followed by postfix. */
> +    js::types::TypeObject *newTypeObject(const char *base, const char *postfix,
> +                                         JSObject *proto, bool isFunction = false);
> +

Why are these methods in JSContext? It seems like the wrong place. And I think in general we are trying to shrink JSContext. Could they be methods of TypeObject? Failing that, the type compartment seems like a better place.

::: js/src/jsemit.cpp
@@ +4443,1 @@
>              return false;

What is this change (and the next one down) for?

::: js/src/jsparse.cpp
@@ +1219,5 @@
>              JSObjectArray *arr = inner->objects();
> +
> +            /* Skip any caller function entrained in the first object. */
> +            size_t start = inner->savedCallerFun ? 1 : 0;
> +

And what about this change? What is it for?

::: js/src/shell/js.cpp
@@ +5892,5 @@
> +     * First check to see if type inference is enabled. This flag must be set
> +     * on the compartment when it is constructed.
> +     */
> +    for (int i = 0; i < argc; i++) {
> +        switch (argv[i][1]) {

This seems unfortunate (the redundancy with the other option-parsing part). I guess the main options part does stuff early, so you want to do this first. But can we get rid of the re

Comment 11

[Brian Hackett [Laid off!]]

(In reply to comment #10)
> > +extern JS_PUBLIC_API(void)
> > +JS_SplicePrototype(JSContext *cx, JSObject *obj, JSObject *proto);
> > +
> 
> How is this API used? Does it have to be public? It seems like an obscure
> usage for JSAPI users.

JS_SplicePrototype behaves like JS_SetPrototype, except that the object it is used on must have a singleton type (no other objects have the same type).  It is operationally very different, which is why I made it a separate function: JS_SetPrototype changes the type of the object itself, and marks the old and new types as completely unknown, whereas JS_SplicePrototype changes the prototype field of the type directly and retains type information.

You might want to also review the DOM/XPCOM changes, which are not very extensive and are where these API changes are used.

> @@ +2702,5 @@
> > +JS_DefineUCFunctionWithType(JSContext *cx, JSObject *obj,
> > +                            const jschar *name, size_t namelen, JSNative call,
> > +                            uintN nargs, uintN attrs,
> > +                            JSTypeHandler handler);
> > +
> 
> In general, on the new *WithType APIs, how are they going to be used? Will
> the browser use them? Will JSAPI users use them? It seems basically
> appropriate to make these public APIs, but I'd like to know more about how
> they work. (Of course, they will also need documentation in MDC.)

I think both should be able to use them.  I've been a little conflicted with these because the WithType is ugly but the only way to allow API users to convey type information to the engine without changing the existing API.  These used to be more important within the engine itself because several months ago we were making a lot more internal calls through the public API, but it looks like those are pretty much gone now (XML notwithstanding, and we won't have type information for XML stuff anyways).  JS_FN_TYPE is also ugly, and used extensively within the engine.

How important is it to avoid breaking API changes, if API users can update their code easily?

> ::: js/src/jscntxt.h
> @@ +1362,5 @@
> > +
> > +    /* Make a type object whose name is that of base followed by postfix. */
> > +    js::types::TypeObject *newTypeObject(const char *base, const char *postfix,
> > +                                         JSObject *proto, bool isFunction = false);
> > +
> 
> Why are these methods in JSContext? It seems like the wrong place. And I
> think in general we are trying to shrink JSContext. Could they be methods of
> TypeObject? Failing that, the type compartment seems like a better place.

The JSContext::newTypeObject and newTypeFunction methods should definitely be methods on TypeCompartment (that's what they wrap, anyways), will fix that.  The idea behind the other type related methods on JSContext and JSScript is to have a common place for methods that gate inference stuff: they are called from code where inference may or may not be enabled, then check if inference is enabled before calling into inference code.  For the most part, TypeCompartment and TypeObject methods shouldn't be called when inference is disabled (exceptions for making skeletal TypeObjects for delegate JSObjects and garbage collection of such TypeObjects).

> 
> ::: js/src/jsemit.cpp
> @@ +4443,1 @@
> >              return false;
> 
> What is this change (and the next one down) for?

Ah, the defineProperty change in the singleton initialization leaked in from the array patch.  We need to preserve initialized length invariants for singleton arrays as they are initialized, and using defineProperty instead of setDenseArrayElement is a clean way to do that.

> ::: js/src/jsparse.cpp
> @@ +1219,5 @@
> >              JSObjectArray *arr = inner->objects();
> > +
> > +            /* Skip any caller function entrained in the first object. */
> > +            size_t start = inner->savedCallerFun ? 1 : 0;
> > +
> 
> And what about this change? What is it for?

This change is no longer necessary, I'll remove it.  Not long ago, we used to try to infer types for NAME related opcodes, which required knowing the static nesting of scripts (we totally punt on NAME opcodes now).  We used this loop to figure out this nesting, and skipped any saved caller function as otherwise we would think the parent was nested in its child.

> ::: js/src/shell/js.cpp
> @@ +5892,5 @@
> > +     * First check to see if type inference is enabled. This flag must be set
> > +     * on the compartment when it is constructed.
> > +     */
> > +    for (int i = 0; i < argc; i++) {
> > +        switch (argv[i][1]) {
> 
> This seems unfortunate (the redundancy with the other option-parsing part).
> I guess the main options part does stuff early, so you want to do this
> first. But can we get rid of the re

Comment is torn.

Comment 12

[David Mandelin [:dmandelin]]

(In reply to comment #11)
> (In reply to comment #10)
> > > +extern JS_PUBLIC_API(void)
> > > +JS_SplicePrototype(JSContext *cx, JSObject *obj, JSObject *proto);
> > > +
> > 
> > How is this API used? Does it have to be public? It seems like an obscure
> > usage for JSAPI users.
> 
> JS_SplicePrototype behaves like JS_SetPrototype, except that the object it
> is used on must have a singleton type (no other objects have the same type).
> It is operationally very different, which is why I made it a separate
> function: JS_SetPrototype changes the type of the object itself, and marks
> the old and new types as completely unknown, whereas JS_SplicePrototype
> changes the prototype field of the type directly and retains type
> information.
> 
> You might want to also review the DOM/XPCOM changes, which are not very
> extensive and are where these API changes are used.
> 
> > @@ +2702,5 @@
> > > +JS_DefineUCFunctionWithType(JSContext *cx, JSObject *obj,
> > > +                            const jschar *name, size_t namelen, JSNative call,
> > > +                            uintN nargs, uintN attrs,
> > > +                            JSTypeHandler handler);
> > > +
> > 
> > In general, on the new *WithType APIs, how are they going to be used? Will
> > the browser use them? Will JSAPI users use them? It seems basically
> > appropriate to make these public APIs, but I'd like to know more about how
> > they work. (Of course, they will also need documentation in MDC.)
> 
> I think both should be able to use them.  I've been a little conflicted with
> these because the WithType is ugly but the only way to allow API users to
> convey type information to the engine without changing the existing API. 
> These used to be more important within the engine itself because several
> months ago we were making a lot more internal calls through the public API,
> but it looks like those are pretty much gone now (XML notwithstanding, and
> we won't have type information for XML stuff anyways).  JS_FN_TYPE is also
> ugly, and used extensively within the engine.
> 
> How important is it to avoid breaking API changes, if API users can update
> their code easily?

I think breaking API changes are OK. I should check on how they affect add-ons, and what we need to do to help add-on authors, but I don't see a problem there.

My main concern here is that the new APIs seem hard to use for normal JSAPI users. (The "WithType" versions don't seem *too* confusing, but JS_SplicePrototype seems dangerous.) I think they should be set off in some way so that people don't get too confused. Or if you do want them to be used by others, they need to be well documented. 

My vote would be to put them into a separate header file, either undocumented (with a brief explanation) as JS_FRIEND_API, or documented as JS_PUBLIC_API.

> > ::: js/src/jscntxt.h
> > @@ +1362,5 @@
> > > +
> > > +    /* Make a type object whose name is that of base followed by postfix. */
> > > +    js::types::TypeObject *newTypeObject(const char *base, const char *postfix,
> > > +                                         JSObject *proto, bool isFunction = false);
> > > +
> > 
> > Why are these methods in JSContext? It seems like the wrong place. And I
> > think in general we are trying to shrink JSContext. Could they be methods of
> > TypeObject? Failing that, the type compartment seems like a better place.
> 
> The JSContext::newTypeObject and newTypeFunction methods should definitely
> be methods on TypeCompartment (that's what they wrap, anyways), will fix
> that.  The idea behind the other type related methods on JSContext and
> JSScript is to have a common place for methods that gate inference stuff:
> they are called from code where inference may or may not be enabled, then
> check if inference is enabled before calling into inference code.  For the
> most part, TypeCompartment and TypeObject methods shouldn't be called when
> inference is disabled (exceptions for making skeletal TypeObjects for
> delegate JSObjects and garbage collection of such TypeObjects).

I see--that makes sense. Adding methods to JSScript and TypeCompartment is fine. But I think JSContext should be a last resort--if you do put things there, we will probably want to move it someday.

> > ::: js/src/shell/js.cpp
> > @@ +5892,5 @@
> > > +     * First check to see if type inference is enabled. This flag must be set
> > > +     * on the compartment when it is constructed.
> > > +     */
> > > +    for (int i = 0; i < argc; i++) {
> > > +        switch (argv[i][1]) {
> > 
> > This seems unfortunate (the redundancy with the other option-parsing part).
> > I guess the main options part does stuff early, so you want to do this
> > first. But can we get rid of the re
> 
> Comment is torn.

I just meant to ask if there is a reasonable way of getting rid of the redundant loops over the switch arrays. I kind of don't want to gate this patch on refactoring options handling in the shell, but I also really want the options handling code in the shell to improve monotonically.

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 532856 [details] [diff] [review]
array changes

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

Seems ok to me, though I didn't check every line with a fine-tooth comb.

Is the plan for type inference to be always on in the long run?  Having all those |if (cx->typeInferenceEnabled())| paths all over the place makes me nervous.

::: js/src/jsarray.cpp
@@ +55,5 @@
>   *
>   * In dense mode, holes in the array are represented by
> + * MagicValue(JS_ARRAY_HOLE) invalid values. Elements between the initialized
> + * length and the length property are left uninitialized, but are conceptually holes.
> + * Arrays with no holes below the initialized length are "packed" arrays.

Is this the main place where "initialized length" is explained?  More explanation is needed.  Things you told me on IRC that should be included in the explanation:

- it is some value which is <= length and <= capacity, where all elements above it are holes

- there's flexibility in exactly the value the initialized length must hold, e.g. you can have an array with length 5, capacity 10, completely empty, it is valid for the initialized length to be any value between zero and 5, as long as the in memory values below the initialized length have in fact been initialized with a hole value

- ie. every slot from 0..(initialized_length - 1) must have been explicitly set, even if set with a hole

A couple of examples would be useful.

Also, I still find this use of "initialized" confusing, I keep wanting it to mean the initial assignment.  Would "explicitly set length" be better?  I don't know.  I'll let you decide on the right name, but give it some thought :)

@@ +498,5 @@
>              if (result != JSObject::ED_OK)
>                  break;
>              if (idx >= obj->getArrayLength())
> +                obj->setDenseArrayLength(idx + 1);
> +            obj->setDenseArrayElementWithType(cx, idx, v);

Why is |cx| needed?  The definition of setDenseArrayElementWithType appears to be missing from this patch.

(I found the definition in the JM repo... make sense now.)

@@ +666,5 @@
>          return true;
>  
>      vp->setNumber(newlen);
>      if (oldlen < newlen) {
> +        obj->setArrayLength(cx, newlen);

Same question: why |cx|?  Where is setArrayLength defined?

@@ +995,5 @@
>  array_trace(JSTracer *trc, JSObject *obj)
>  {
>      JS_ASSERT(obj->isDenseArray());
>  
> +    uint32 capacity = obj->getDenseArrayInitializedLength();

Rename |capacity| as |initLength|.

::: js/src/jsarray.h
@@ +52,5 @@
>  /* Small arrays are dense, no matter what. */
>  const uintN MIN_SPARSE_INDEX = 256;
>  
> +inline uint32
> +JSObject::getDenseArrayInitializedLength()

I know the |getFoo()| style has been used for arrays previously (I used it myself :), but apparently |foo()| is preferred;  the 'get' prefix in SM usually means it's fallible in some way.

@@ +74,5 @@
> +    return flags & PACKED_ARRAY;
> +}
> +
> +inline void
> +JSObject::setDenseArrayNotPacked(JSContext *cx)

Would markDenseAraryAsNotPacked be a better name?

@@ +109,5 @@
> +                setDenseArrayNotPacked(cx);
> +                ClearValueRange(getDenseArrayElements() + initLength,
> +                                index - initLength, true);
> +            }
> +            setDenseArrayInitializedLength(index + 1);

These few lines appear three times in this function.  Please factor them out into a separate function.

::: js/src/jsiter.cpp
@@ +79,3 @@
>  #include "jsobjinlines.h"
>  #include "jsstrinlines.h"
> +#include "jsautooplen.h"

Do you need to add these two new #includes?  I don't see anything in the rest of this file that would appear to need them.

@@ +83,5 @@
>  #include "vm/Stack-inl.h"
>  
>  using namespace js;
>  using namespace js::gc;
> +using namespace js::types;

Similarly, is this necessary?

::: js/src/jsobj.cpp
@@ +2455,5 @@
>              return Reject(cx, obj, JSMSG_CANT_DEFINE_ARRAY_INDEX, throwError, rval);
>  
>          if (index >= oldLen) {
>              JS_ASSERT(index != UINT32_MAX);
> +            obj->setArrayLength(cx, index + 1);

Why does setArrayLength() need 'cx'?  The patch seems to be missing the change to setArrayLength() so I can't tell.

::: js/src/jsobj.h
@@ +858,5 @@
>      inline void shrinkDenseArrayElements(JSContext *cx, uintN cap);
> +    inline bool denseArrayHasInlineSlots() const;
> +    inline void backfillDenseArrayHoles();
> +
> +    /* Packed information for this array. May be incorrect if !cx->typeInferenceEnabled(). */

Presumably these functions should not be called if !cx->typeInferenceEnabled()?  If so, please add assertions in them to this effect, and change this comment to "should be not called if !cx->typeInferenceEnabled()".

::: js/src/jstracer.cpp
@@ +13309,5 @@
> +         * Grow the array and fully initialize it if the index exceeds the initialized
> +         * length of the array.  This happens rarely, eg. less than 1% of the time
> +         * in SunSpider.
> +         */
> +        LIns* initlen_ins = w.ldiDenseArrayInitializedLength(obj_ins);

Looks like this patch is missing a change to tracejit/Writer.h?

Comment 14

[Brian Hackett [Laid off!]]

(In reply to comment #13)
> Is the plan for type inference to be always on in the long run?  Having all
> those |if (cx->typeInferenceEnabled())| paths all over the place makes me
> nervous.

Eventually I think this might be the case, but at least for a while we'll want to be able to compare behavior with inference enabled/disabled.

> @@ +666,5 @@
> >          return true;
> >  
> >      vp->setNumber(newlen);
> >      if (oldlen < newlen) {
> > +        obj->setArrayLength(cx, newlen);
> 
> Same question: why |cx|?  Where is setArrayLength defined?

The context is needed here because we will need to update the array's type information if the length overflows INT32_MAX --- we need to know accessing the 'length' property may now produce a double.  The definition for setArrayLength is in this patch, in jsobjinlines.h (sorry about missing other functions used by the diff, I think some stuff got folded into the object changes patch).

Comment 15

[Brian Hackett [Laid off!]]

(In reply to comment #12)
> I think breaking API changes are OK. I should check on how they affect
> add-ons, and what we need to do to help add-on authors, but I don't see a
> problem there.
> 
> My main concern here is that the new APIs seem hard to use for normal JSAPI
> users. (The "WithType" versions don't seem *too* confusing, but
> JS_SplicePrototype seems dangerous.) I think they should be set off in some
> way so that people don't get too confused. Or if you do want them to be used
> by others, they need to be well documented. 
> 
> My vote would be to put them into a separate header file, either
> undocumented (with a brief explanation) as JS_FRIEND_API, or documented as
> JS_PUBLIC_API.

I removed the WithType variations from jsapi.h moved SplicePrototype and NewObjectWithUniqueType into jsfriendapi.h (as JS_FRIEND_API functions).  The patch is here (pretty straightforward):

http://hg.mozilla.org/projects/jaegermonkey/rev/0cc71b0c30f4

Comment 16

[Brian Hackett [Laid off!]]

Attachment: arguments patch

Arguments patch from bug 658638, which is mostly analysis/compiler internal but changes the interpreter somewhat.  Luke, do you mind reviewing the VM changes here?

Can people please finish the remaining reviews within the next few days?  I would really, really like to get TI landed to TM soon.  Barring complications, I'm not going to be doing anything on this branch besides bug fixing before it lands.

Comment 17

[David Mandelin [:dmandelin]]

(In reply to comment #15)
> I removed the WithType variations from jsapi.h moved SplicePrototype and
> NewObjectWithUniqueType into jsfriendapi.h (as JS_FRIEND_API functions). 
> The patch is here (pretty straightforward):
> 
> http://hg.mozilla.org/projects/jaegermonkey/rev/0cc71b0c30f4

Can you post this as a patch here for me to review?

Comment 18

[David Mandelin [:dmandelin]]

Comment on attachment 532858 [details] [diff] [review]
XPCOM/DOM changes

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

::: js/src/xpconnect/src/xpcprivate.h
@@ +4406,5 @@
>  // Wrapper for JS_NewObject to mark the new object as system when parent is
>  // also a system object.
>  inline JSObject*
>  xpc_NewSystemInheritingJSObject(JSContext *cx, JSClass *clasp, JSObject *proto,
> +                                bool uniqueType, JSObject *parent);

Please add a comment documenting the requirements on the |uniqueType| parameter.

I'll also need that to properly review the changes to the call sites of this API. Everything else looks OK so I should be able to turn this one around quickly once a revised patch is up.

Comment 19

[Brian Hackett [Laid off!]]

Attachment: reverted API changes

Patch from rev 0cc71b0c30f4.  The entire patch is here, but note that some of the changes (all the jsperf.cpp changes, some of jsxml.cpp) are just reverting code to their state in the TM branch.

Comment 20

[Brian Hackett [Laid off!]]

Attachment: comment for xpc_NewSystemInheritingJSObject

Comment for xpc_NewSystemInheritingJSObject.  For background, the uniqueType flag (and corresponding friend API method) were added to handle cases where XPConnect constructs objects which are put on the window's prototype chain.  If generic types are used for these objects then their property types are unknown, imprecision which propagates down into the window itself.

Comment 21

[David Mandelin [:dmandelin]]

Comment on attachment 532857 [details] [diff] [review]
type hooks and API changes

On this patch, what about moving methods out of JSContext? It is not critical, but I think we want to move in the direction of simplifying and shrinking JSContext, so it would be good to do that now.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 532854 [details] [diff] [review]
object changes

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

Mostly, I just have a lot of questions about what's going on in the code. In all these places, it would be nice to have some explanatory comments. I'm holding back r+ing the patch until the GC marking stuff is addressed.

Here are my comments about jstracer.cpp, which are being garbled by a bug in Splinter:

It would be nice if stobj_set_dslot did the obj->dynamicSlotIndex(slot) rather than its callers. I realize this means that the call from TraceRecorder::newArray would have to change, so maybe we should also have stobj_set_arrayslot, or something like that.

In the same vein, it would be nice to split DSlotsAddress into DSlotsAddress and ArraySlotsAddress. DSlotsAddress would automatically do the obj->dynamicSlotIndex(slot) so that you'd be less likely to forget to do it. This would require some fixups in the tracer to split apart the dslots cases from the array slots cases, but I think it would result in clearer code.

::: js/src/jsfun.cpp
@@ +2835,5 @@
>      if (cx->compartment == fun->compartment()) {
>          /*
> +         * Don't clone functions with singleton types, we need to ensure that
> +         * there is only one object with this type. We may need to reparent the
> +         * function, however.

It would be good to say more why this is safe to do.

::: js/src/jsgcinlines.h
@@ +126,5 @@
>  {
>      extern FinalizeKind slotsToThingKind[];
>  
>      if (numSlots >= SLOTS_TO_THING_KIND_LIMIT)
> +        return FINALIZE_OBJECT16;

Why the change here?

::: js/src/jsgcmark.cpp
@@ +526,5 @@
>  
>      if (JSObject *parent = obj->getParent())
>          PushMarkStack(gcmarker, parent);
> +    if (!obj->isDenseArray() && obj->newType && !obj->newType->marked)
> +        obj->newType->trace(gcmarker);

Now that you're planning to merge this stuff, I'd really like this to be done with a fast path similar to ScanShape. The problem is that TypeObject::trace calls functions like MarkObject and MarkShape. It's faster to call PushMarkStack, since it skips a couple unnecessary checks and it also allows the compiler to inline.

I realize that it's annoying to duplicate this path, but I think it's worth it.

@@ +727,5 @@
>      }
>  }
> +
> +void
> +JSObject::markSlots(JSTracer *trc)

Can't markSlots use a single loop that calls getSlot? It's slower, but markSlots is a slow path. The fast path is ScanObject, where I think it would make sense to replace the getSlot call with two loops, since this might save a bunch of branches.

::: js/src/jsobj.cpp
@@ +3003,5 @@
> +        JS_ASSERT(cx->typeInferenceEnabled());
> +
> +        static unsigned count = 0;
> +        char *name = (char *) alloca(30);
> +        JS_snprintf(name, 30, "SpecializedThis:%u", ++count);

Shouldn't this be #ifdef DEBUG?

@@ +3009,5 @@
> +        types::TypeObject *type = cx->newTypeObject(name, obj->getProto());
> +        types::AutoTypeRooter root(cx, type);
> +
> +        obj = NewReshapedObject(cx, type, obj->getParent(), gc::FinalizeKind(obj->finalizeKind()),
> +                                obj->lastProperty());

Could you explain this? It looks really scary.

@@ +3639,5 @@
> +
> +        if (a->isNative())
> +            a->generateOwnShape(cx);
> +        if (b->isNative())
> +            b->generateOwnShape(cx);

What's the purpose of this?

@@ +3680,5 @@
> +        }
> +
> +        /* From here the swap is infallible. */
> +
> +        /* Wipe out any dynamic slots, we will reallocate if needed. */

What does "we will reallocate if needed" mean?

@@ +3686,5 @@
> +            cx->free_(a->slots);
> +        if (b->hasSlotsArray())
> +            cx->free_(b->slots);
> +
> +        JSObject tmp;

It might be good to assert here that neither operand is a JSFunction.

@@ +3691,5 @@
> +        memcpy(&tmp, a, sizeof tmp);
> +        memcpy(a, b, sizeof tmp);
> +        memcpy(b, &tmp, sizeof tmp);
> +
> +        a->flags = (a->flags & ~FIXED_SLOTS_MASK) | (afixed << FIXED_SLOTS_SHIFT);

Could you use getters and setters here to make this a little cleaner?

@@ +3740,5 @@
>      }
> +    if (!TradeGuts(cx, this, otherClone))
> +        return false;
> +    if (!TradeGuts(cx, other, thisClone))
> +        return false;

What will happen if the second one fails? Won't we end up in a state where this == other?

@@ +4324,5 @@
> +        unsigned newScriptSlots = gc::GetGCKindSlots(gc::FinalizeKind(type->newScript->finalizeKind));
> +        if (newScriptSlots == numFixedSlots()) {
> +            /*
> +             * Bump by two to keep BACKGROUND consistent.
> +             * :XXX: this FINALIZE_*_BACKGROUND stuff needs an abstraction.

Yeah, abstraction would be great. Could you add functions to jsgcinlines for this and also for when we do the +1 elsewhere?

@@ +4329,5 @@
> +             */
> +            unsigned newKind = type->newScript->finalizeKind + 2;
> +            if (newKind <= gc::FINALIZE_OBJECT_LAST) {
> +                JSObject *obj = NewReshapedObject(cx, type, getParent(),
> +                                                  gc::FinalizeKind(newKind), type->newScript->shape);

Again, it seems really scary to me to be allocating a whole object. Couldn't we get some really bad worst-case behavior? Could you abstract the shape creation out of the NewObject code and just use that?

@@ +4401,5 @@
>      /* If nothing was allocated yet, treat it as initial allocation. */
>      if (!hasSlotsArray())
>          return allocSlots(cx, actualCapacity);
>  
> +    uint32 oldAllocCount = isDenseArray() ? oldcap : oldcap - numFixedSlots();

Could this be factored into a helper, such as numDynamicSlots or something? You could compute the old one before allocSlots and the other after.

@@ +4418,5 @@
> +        ClearValueRange(slots + oldAllocCount, allocCount - oldAllocCount, false);
> +    }
> +
> +    if (changed && isGlobal())
> +        cx->markGlobalReallocation(this);  /* :XXX: Handle failure? */

Comments like this are frightening to anyone who comes across them. Could you either resolve this issue or explain it more fully?

@@ +4430,5 @@
>      uint32 oldcap = numSlots();
>      JS_ASSERT(newcap <= oldcap);
>      JS_ASSERT(newcap >= slotSpan());
>  
> +    size_t fixed = numFixedSlots();

Is it important that this be computed up here? If not, could you move it closer to where it's used.

@@ +4463,5 @@
> +            clearSlotRange(fill, newcap - fill);
> +    }
> +
> +    if (changed && isGlobal())
> +        cx->markGlobalReallocation(this);  /* :XXX: Handle failure? */

Again, comment needs explaining.

::: js/src/jsobj.h
@@ +316,5 @@
> + * For objects other than dense arrays, if the object has N fixed slots then
> + * those are always the first N slots of the object. The dynamic slots pointer
> + * is used if those fixed slots overflow, and stores all remaining slots.
> + * Unlike dense arrays, the fixed slots can always be accessed. Two objects
> + * with the same shape have the same number of fixed slots.

Maybe state explicitly that the slots pointer won't point to fixedSlots() when there are no dynamic slots.

@@ +399,2 @@
>  
> +        /* If dense array, initialized length of the array. */

Please explain the term "initialized length" more fully.

::: js/src/jsobjinlines.h
@@ +805,5 @@
> +{
> +    if (isDenseArray() && !makeDenseArraySlow(cx))
> +        return NULL;
> +    if (newType) {
> +        if (newType->newScript && newType->newScript->script != script)

Please add a comment about what's going on here.

@@ +874,5 @@
> +}
> +
> +inline void
> +JSObject::init(JSContext *cx, js::Class *aclasp, js::types::TypeObject *type,
> +               JSObject *parent, void *priv, bool useHoles)

I feel like the useHoles parameter would benefit from a better name.

@@ +906,4 @@
>  
> +    newType = NULL;
> +    JS_ASSERT(initializedLength == 0);
> +        initializedLength = 0;

Extra spaces.

::: js/src/jsscope.cpp
@@ -1208,5 @@
> -        if (shape->slot == fixed) {
> -            JS_ASSERT_IF(!lastProp->isEmptyShape() && lastProp->hasSlot(),
> -                         lastProp->slot == fixed - 1);
> -            revertToFixedSlots(cx);
> -        }

Is there a reason why you don't free the dynamic slots in this case? It seems like the GC will take care of it, but it would be nice if we could do it eagerly.

@@ -1255,5 @@
> -     * allocated slot, preserving invariant that objects with the same shape
> -     * use the fixed slots in the same way.
> -     */
> -    if (hasSlotsArray() && JSSLOT_FREE(getClass()) <= numFixedSlots())
> -        revertToFixedSlots(cx);

Same question here.

::: js/src/tracejit/Writer.cpp
@@ +266,5 @@
>      return ret;
>  }
>  
>  static bool
>  isConstPrivatePtr(LIns *ins, unsigned slot)

It seems like this also is only used for reserved slots, so maybe a name change would be good here.

@@ +270,5 @@
>  isConstPrivatePtr(LIns *ins, unsigned slot)
>  {
> +    // match both inline and indirect slot accesses.
> +    uint32 inlineOffset = JSObject::getFixedSlotOffset(slot) + sPayloadOffset;
> +    uint32 oolOffset = (slot * sizeof(Value)) + sPayloadOffset;

If this is always a reserved slot, then shouldn't we always be hitting the inline case? If not, please explain.

@@ +310,5 @@
>  {
>      bool ok;
>  
> +    // accesses to ACCSET_NONE are on immutable state
> +    if (accSet == ACCSET_NONE)

What caused this change to be necessary?

@@ +407,5 @@
>        // base = <JSObject>
>        // ins  = ldp.obj<field> base[offsetof(JSObject, <field>)]
>        #define OK_OBJ_FIELD(ldop, field) \
> +            ((op == (ldop)) && \
> +            (disp == offsetof(JSObject, field)) && \

I think you only need the extra outer parens.

::: js/src/tracejit/Writer.h
@@ +1186,2 @@
>       */
>      nj::LIns *getObjPrivatizedSlot(nj::LIns *obj, uint32 slot) const {

It seems like this is always used to load reserved slots. Could we maybe change the name?

Comment 23

[Luke Wagner [:luke]]

Comment on attachment 532849 [details] [diff] [review]
stack layout changes

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

Overall looks good but, as with everyone else, I can't say I understand it all; js_InternalInterpret made my head explode.  Hopefully in IonMonkey we can do something cleaner?

Reminder from IRC: the (f->stubRejoin & 0x1) in Recompiler::expandInlineFrames seems wrong since this is before stubRejoin is munged.

Mostly nits:

::: js/src/jscntxt.cpp
@@ +1389,5 @@
>  StackFrame *
>  js_GetScriptedCaller(JSContext *cx, StackFrame *fp)
>  {
>      if (!fp)
> +        fp = js_GetTopStackFrame(cx, FRAME_EXPAND_NONE);

I think this one needs FRAME_EXPAND_TOP; the returned frame's script is used (to get the filename to associate with errors and eval code etc).

::: js/src/jscntxtinlines.h
@@ +57,5 @@
> +    FrameRegs *prevContextRegs;
> +    PreserveRegsGuard(JSContext *cx, FrameRegs &regs)
> +      : cx(cx), regs(regs), prevContextRegs(cx->maybeRegs()) {
> +        cx->stack.repointRegs(&regs);
> +    }

There is some subtle naming going on here re: this->regs and the 'regs' arg.  Perhaps make members private and give trailing _?

::: js/src/jsdbgapi.cpp
@@ +1371,5 @@
>  JS_PUBLIC_API(JSStackFrame *)
>  JS_FrameIterator(JSContext *cx, JSStackFrame **iteratorp)
>  {
>      StackFrame *fp = Valueify(*iteratorp);
> +    *iteratorp = Jsvalify((fp == NULL) ? js_GetTopStackFrame(cx, FRAME_EXPAND_NONE) : fp->prev());

Though we eventually need to kill this, its used for all sorts of things (like firebug) that probably want all frames, so FRAME_EXPAND_ALL.

::: js/src/jsexn.cpp
@@ +294,5 @@
>  
>      callerid = ATOM_TO_JSID(cx->runtime->atomState.callerAtom);
>      stackDepth = 0;
>      valueCount = 0;
> +    for (fp = js_GetTopStackFrame(cx, FRAME_EXPAND_NONE); fp; fp = fp->prev()) {

Since this isn't a hot path, it seems like users would enjoy getting all their frames in the exception callstack.

@@ +337,5 @@
>      priv->stackDepth = stackDepth;
>  
>      values = GetStackTraceValueBuffer(priv);
>      elem = priv->stackElems;
> +    for (fp = js_GetTopStackFrame(cx, FRAME_EXPAND_NONE); fp != fpstop; fp = fp->prev()) {

This would need to be fixed to match.

@@ +697,5 @@
>      JSString *message, *filename;
>      StackFrame *fp;
>  
> +#ifdef JS_METHODJIT
> +    js::mjit::ExpandInlineFrames(cx, true);

Is there somewhere between here and the js_GetTopStackFrame called by InitExceptionPrivate (below) that needs the reified stack?  If not, perhaps this can be removed.  Really, here and for all other uses of ExpandInlineFrames, it would be nice if we followed the tracer's lead and funnel everything through js_GetTopStackFrame.  There are several exceptions, but if we put ExpandInlineFrames in AllFramesIter() and Recompiler::recompile() I think the rest would be redundant.

::: js/src/jsfun.cpp
@@ +1611,5 @@
> +         * frames on the stack before we go looking for them.
> +         */
> +        if (fun->isInterpreted())
> +            cx->markTypeObjectFlags(fun->getType(), OBJECT_FLAG_UNINLINEABLE);
> +    }

It would be useful here (and other places throughout the vm where there are these assorted type-inference guards to state what assumption type inference is making that is being violated here (more than just "there is no f.caller/f.arguments" :).

@@ +1629,5 @@
> +        fp->prev()->pc(cx, fp, &inlined);
> +        if (inlined) {
> +            JSFunction *fun = fp->prev()->jit()->inlineFrames()[inlined->inlineIndex].fun;
> +            cx->markTypeObjectFlags(fun->getType(), OBJECT_FLAG_UNINLINEABLE);
> +        }

My initial question was: "why not mark the caller uninlineable if it isn't currently inlined (but might be in the future)?"  The comment about what exactly TI depends on would be useful for this.

::: js/src/jsinterp.cpp
@@ +111,5 @@
>  
>  JSObject *
>  js::GetScopeChain(JSContext *cx)
>  {
> +    StackFrame *fp = js_GetTopStackFrame(cx, FRAME_EXPAND_NONE);

Perhaps a comment to the effect of "calls are only inlined which have the same scope chain as the outer (non-inlined) frame".  Also asserting !fp->hasCallObj() would be nice.

::: js/src/jsobj.cpp
@@ +3112,5 @@
>      jsbytecode *endpc;
>      JSOp op;
>      JSAtom *atom;
>  
> +    cx->fp()->inlinepc(cx, &script);

There's are 5 instances of cx->fp()->inlinepc.  As part of the general effort to get away from stack frames and instead use more semantically-targeted queries, could we change all these patterns to:

  JSScript *script = cx->stack.currentScript()

(Just like one currently asks for "cx->stack.fp()".)  That leaves two uses of inlinepc + js_GetScriptedCaller (both in jsinferinlines.h).  Since js_GetScriptedCaller is already being changed to expand more (from above comment), perhaps you can just make a more specific function to replace caller->inlinepc altogether and allow it to be removed.

@@ +5368,5 @@
>      int scopeIndex;
>      JSProperty *prop;
>  
>      JS_ASSERT_IF(cacheResult, !JS_ON_TRACE(cx));
> +    scopeChain = &js_GetTopStackFrame(cx, FRAME_EXPAND_NONE)->scopeChain();

This would also want the same comment as above.  Having more than one instance of this indicates that perhaps we should have:

  JSObject &scopeChain = cx->stack.currentScriptedScopeChain();

("Scripted" since in view of bug 631135, this query doesn't take into account natives changing globals.)

::: js/src/jspropertycache.cpp
@@ +152,5 @@
>                  break;
>              }
>  
> +            if (!pobj->generic() && shape->hasDefaultGetter() && pobj->containsSlot(shape->slot) &&
> +                !cx->typeInferenceEnabled()) {

Comment would be good.  Such scattered checks are scary to those unfamiliar with TI invariants.

::: js/src/jsxml.cpp
@@ +7444,5 @@
>      qn = nameobj;
>      if (!IsFunctionQName(cx, qn, &funid))
>          return JS_FALSE;
>  
> +    obj = &js_GetTopStackFrame(cx, FRAME_EXPAND_NONE)->scopeChain();

cx->stack.currentScriptedScopeChain()

::: js/src/vm/Stack-inl.h
@@ +788,5 @@
>  inline Value *
>  StackSpace::getStackLimit(JSContext *cx)
>  {
>      FrameRegs &regs = cx->regs();
> +    uintN minSpace = regs.fp()->numSlots() + STACK_EXTRA;

+ VALUES_PER_STACK_FRAME?

@@ +855,5 @@
> +            /*
> +             * The current regs.pc may not be intact, set it in case bumping
> +             * the limit fails.
> +             */
> +            cx->regs().pc = mjit::NativeToPC(cx->fp()->jit(), topncode);

Instead of putting this here, could the error handling code detect that it needs to fixup pc by the presence of inline frames?  It seems like it would have to do this already...

@@ +874,5 @@
>      JS_ASSERT(fun->script() == script);
>      JS_ASSERT(space().firstUnused() == firstUnused);
>  
> +    /* Include extra space for inlining by the method-jit. */
> +    uintN nvals = StackSpace::STACK_EXTRA + script->nslots;

Why is STACK_EXTRA necessary here: if the methodjit is calling, shouldn't it have Check = LimitCheck?

::: js/src/vm/Stack.h
@@ +443,5 @@
> +     * not reflect any inlining that has been performed by the method JIT.
> +     *
> +     * If other frames were inlined into this one, the script/pc reflect the
> +     * point of the outermost call. Use inlinepc to get the script/pc for
> +     * the innermost inlined frame. Inlined frame invariants:

With the above comments addressed, inlinepc() can go away and this comment can be moved to the new ContextStack::currentScript().

@@ +446,5 @@
> +     * point of the outermost call. Use inlinepc to get the script/pc for
> +     * the innermost inlined frame. Inlined frame invariants:
> +     *
> +     * - Inlined frames have the same scope chain as the outer frame.
> +     * - Inlined frames have the same strictness as the outer frame.

Ah, this is the comment I was interested above.  It seems like it could go on the proposed new ContextStack::currentScriptedScopeChain.

@@ +1025,4 @@
>      StackFrame *fp_;
>    public:
>      StackFrame *fp() const { return fp_; }
> +    JSInlinedSite *inlined() const { return inlined_; }

IIUC, if !!inlined_, we shouldn't be touching pc.  It would be good to assert this by making pc private and having a public member pc() that asserts !inlined_.  Is this too big of a change?

@@ +1181,5 @@
>      /*
> +     * Extra space to reserve on the stack before invoking the method JIT.
> +     * This may be used for inlined stack frames.
> +     */
> +    static const size_t STACK_EXTRA = (VALUES_PER_STACK_FRAME + 18) * 10;

Perhaps a more specific s/STACK_EXTRA/JIT_INLINE_FRAME_SPACE/ ?  The constants "18" and "10" seem like they should get symbols so they can be used by the mjit.

Comment 24

[Brian Hackett [Laid off!]]

(In reply to comment #22)
> ::: js/src/jsgcinlines.h
> @@ +126,5 @@
> >  {
> >      extern FinalizeKind slotsToThingKind[];
> >  
> >      if (numSlots >= SLOTS_TO_THING_KIND_LIMIT)
> > +        return FINALIZE_OBJECT16;
> 
> Why the change here?

I partially reverted this change this morning in rev 8a0b550c10eb, see below.  This was slowing down construction of arrays with length > 16 when inference is disabled (we would fill in the 16 slots with holes, then convert to dynamic slots).

 /* Get the best kind to use when making an object with the given slot count. */
 static inline FinalizeKind
-GetGCObjectKind(size_t numSlots)
+GetGCObjectKind(size_t numSlots, bool isArray = false)
 {
     extern FinalizeKind slotsToThingKind[];
 
-    if (numSlots >= SLOTS_TO_THING_KIND_LIMIT)
-        return FINALIZE_OBJECT16;
+    if (numSlots >= SLOTS_TO_THING_KIND_LIMIT) {
+        /*
+         * If the object will definitely want more than the maximum number of
+         * fixed slots, use zero fixed slots for arrays and the maximum for
+         * other objects. Arrays do not use their fixed slots anymore when
+         * they have a slots array, while other objects will continue to do so.
+         */
+        return isArray ? FINALIZE_OBJECT0 : FINALIZE_OBJECT16;
+    }
     return slotsToThingKind[numSlots];
 }

> ::: js/src/jsgcmark.cpp
> @@ +526,5 @@
> >  
> >      if (JSObject *parent = obj->getParent())
> >          PushMarkStack(gcmarker, parent);
> > +    if (!obj->isDenseArray() && obj->newType && !obj->newType->marked)
> > +        obj->newType->trace(gcmarker);
> 
> Now that you're planning to merge this stuff, I'd really like this to be
> done with a fast path similar to ScanShape. The problem is that
> TypeObject::trace calls functions like MarkObject and MarkShape. It's faster
> to call PushMarkStack, since it skips a couple unnecessary checks and it
> also allows the compiler to inline.
> 
> I realize that it's annoying to duplicate this path, but I think it's worth
> it.

The expectation here is that we create few type objects in relation to the number of shapes or (especially) objects, so that using the mark stack when handling type objects wouldn't really make a perf difference.  I haven't measured though, what is the best way to do so?  I really don't like adding complexity without evidence it is necessary.

> @@ +727,5 @@
> >      }
> >  }
> > +
> > +void
> > +JSObject::markSlots(JSTracer *trc)
> 
> Can't markSlots use a single loop that calls getSlot? It's slower, but
> markSlots is a slow path. The fast path is ScanObject, where I think it
> would make sense to replace the getSlot call with two loops, since this
> might save a bunch of branches.

Makes sense, I think this code predates the mark stack changes.

> @@ +3009,5 @@
> > +        types::TypeObject *type = cx->newTypeObject(name, obj->getProto());
> > +        types::AutoTypeRooter root(cx, type);
> > +
> > +        obj = NewReshapedObject(cx, type, obj->getParent(), gc::FinalizeKind(obj->finalizeKind()),
> > +                                obj->lastProperty());
> 
> Could you explain this? It looks really scary.

The comments for the newType stuff are in UseNewType in jsinfer.cpp (need a link to that here).  This could get pathological behavior if someone wrote a hot loop which has 'Foo.prototype = new Bar()' or something in the body, can wait and see if this ever shows up.

> @@ +3639,5 @@
> > +
> > +        if (a->isNative())
> > +            a->generateOwnShape(cx);
> > +        if (b->isNative())
> > +            b->generateOwnShape(cx);
> 
> What's the purpose of this?

The layout of fixed slots for a and b are about to change, so we need to make sure they have their own shapes and that jitcode can continue to assume two objects with the same shape have the same number of fixed slots.

> @@ +3686,5 @@
> > +            cx->free_(a->slots);
> > +        if (b->hasSlotsArray())
> > +            cx->free_(b->slots);
> > +
> > +        JSObject tmp;
> 
> It might be good to assert here that neither operand is a JSFunction.

There is an assertion at the top of TradeGuts which will catch this.

> @@ +3740,5 @@
> >      }
> > +    if (!TradeGuts(cx, this, otherClone))
> > +        return false;
> > +    if (!TradeGuts(cx, other, thisClone))
> > +        return false;
> 
> What will happen if the second one fails? Won't we end up in a state where
> this == other?

Ooh, good catch.  I'll change this so that all resources are acquired ahead of time and these become infallible.

> @@ +4329,5 @@
> > +             */
> > +            unsigned newKind = type->newScript->finalizeKind + 2;
> > +            if (newKind <= gc::FINALIZE_OBJECT_LAST) {
> > +                JSObject *obj = NewReshapedObject(cx, type, getParent(),
> > +                                                  gc::FinalizeKind(newKind), type->newScript->shape);
> 
> Again, it seems really scary to me to be allocating a whole object. Couldn't
> we get some really bad worst-case behavior? Could you abstract the shape
> creation out of the NewObject code and just use that?

The problem is that we aren't just going through the NewObject empty shape path, but reshaping the entire property lineage of the original object for the new finalize kind.  This could be factored out of JSObject::putProperty but wouldn't make anything faster.  This code runs at most five times per type object, so will only be a problem if we start creating tons of type objects (which indicates a deeper problem).

> @@ +4418,5 @@
> > +        ClearValueRange(slots + oldAllocCount, allocCount - oldAllocCount, false);
> > +    }
> > +
> > +    if (changed && isGlobal())
> > +        cx->markGlobalReallocation(this);  /* :XXX: Handle failure? */
> 
> Comments like this are frightening to anyone who comes across them. Could
> you either resolve this issue or explain it more fully?

This comment (and the similar one below) were stale and are removed in the latest.  All inference hooks are now infallible so there is no need to handle failure here.

> ::: js/src/jsscope.cpp
> @@ -1208,5 @@
> > -        if (shape->slot == fixed) {
> > -            JS_ASSERT_IF(!lastProp->isEmptyShape() && lastProp->hasSlot(),
> > -                         lastProp->slot == fixed - 1);
> > -            revertToFixedSlots(cx);
> > -        }
> 
> Is there a reason why you don't free the dynamic slots in this case? It
> seems like the GC will take care of it, but it would be nice if we could do
> it eagerly.

The revertToFixedSlots was needed before the slots reorganization to preserve the invariant that shape determines whether an object uses fixed slots or not.  This functionality isn't needed anymore so I cut it completely to reduce complexity; this code is really cold.

> @@ +310,5 @@
> >  {
> >      bool ok;
> >  
> > +    // accesses to ACCSET_NONE are on immutable state
> > +    if (accSet == ACCSET_NONE)
> 
> What caused this change to be necessary?

The access set was marked as NONE for loading type->proto, which is normally immutable but actually can now be changed as the program is running due to DOM stuff.  Will add the assert back and change the access set for type->proto.

Comment 25

[Brian Hackett [Laid off!]]

(In reply to comment #23)
> Overall looks good but, as with everyone else, I can't say I understand it
> all; js_InternalInterpret made my head explode.  Hopefully in IonMonkey we
> can do something cleaner?

The complexity of interpreter rejoins is basically a function of how much the jitcode's behavior deviates from the interpreter itself.  If jitcode stubcalls always finish up the current opcode before returning, then rejoining would be simple, but when the stub rejoins into an inconsistent interpreter state we need to fix this up.

This complexity pretty much all comes from opcode splitting and fusing in the compiler (particularly incops) and from lowered call/apply.  I'm hoping that in IonMonkey we'll be able to fix the former by changing/simplifying the interpreter to match the compiler's behavior, and to fix the latter by leveraging type information to simplify call paths.

Comment 26

[Brian Hackett [Laid off!]]

Attachment: Move inference methods out of JSContext

This turns the inference methods on JSContext (except typeInferenceEnabled()) into global methods in the js::types namespace which take a context parameter.

Comment 27

[Brian Hackett [Laid off!]]

Attachment: GC mark changes

This moves TypeObject::trace into jsgcmark.cpp and uses a new InlineMark to mark the type object's contents.  This is a lighter weight version of Mark for cases where the object is known to be in the right compartment and which can be inlined.

Comment 28

[Luke Wagner [:luke]]

Comment on attachment 536371 [details] [diff] [review]
arguments patch

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

::: js/src/jsdbgapi.cpp
@@ +225,5 @@
>          script->debugMode = !!debug;
> +
> +        /* Mark arguments objects as escaping in all scripts if debug mode is on. */
> +        if (script->usesArguments && debug)
> +            cx->markTypeObjectFlags(script->fun->getType(), types::OBJECT_FLAG_CREATED_ARGUMENTS);

Why is this necessary again?  If eval-in-frame creates an args object where none is expected, then it will still go through js_GetArgsValue/Object and set the mark / reify args objects.  Is there another way args objects are observed that debug mode allows?  If so, then updating the comment to explain would be good.

::: js/src/jsemit.cpp
@@ +2823,5 @@
>          if (pn2->pn_type == TOK_NAME) {
>              /* Try to optimize arguments.length into JSOP_ARGCNT */
>              if (!BindNameToSlot(cx, cg, pn2))
>                  return JS_FALSE;
> +            if (!cx->typeInferenceEnabled() &&

Perhaps a short comment to the effect of "type inference optimizes this separately".

::: js/src/jsfun.cpp
@@ +109,5 @@
>  {
>      JSObject *argsobj;
>  
> +    cx->markTypeObjectFlags(fp->fun()->getType(),
> +                            OBJECT_FLAG_CREATED_ARGUMENTS | OBJECT_FLAG_UNINLINEABLE);

From IRL: we should move this check to js_GetArgsObject so that we don't have to worry about paths to js_GetArgsObject that don't mark CREATED_ARGUMENTS.  Currently, this only seems possible through the JSAPI (via JS_GetScopeChain/JS_GetFrameCallObject etc).

::: js/src/jsinfer.cpp
@@ +1604,5 @@
> +#ifdef JS_METHODJIT
> +    mjit::ExpandInlineFrames(cx, FRAME_EXPAND_ALL);
> +#endif
> +
> +    /* :FIXME: handle OOM at calls here. */

Still FIXME here and below.

::: js/src/jsinterp.cpp
@@ +5180,5 @@
> +            AutoEnterTypeInference enter(cx);
> +            analysis->analyzeTypes(cx);
> +        }
> +        if (!analysis || analysis->OOM())
> +            goto error;

This hunk o' code doesn't seem specific to JSOP_ARGUMENTS and I think it would be less noisy and reusable to add a 'ensureRanInference' helper so that this code could just be written:

  if (!script->ensureRanInference(cx))
    goto error;

(This helper could also be reused in FixLazyArguments.)  The overall goal is to minimize open-coded TI logic scattered through the VM. 

As a general comment: it would be great if we could get rid of this OOM() state-checking and instead use return-values to indicate OOM in the general SpiderMonkey style.  I know OOM-states show up in the tracer and mjit, but that is b/c of the creepy ballast scheme we use that I don't think is at play here.

::: js/src/methodjit/StubCalls.cpp
@@ +450,5 @@
> +            return;
> +        }
> +        cx->markTypeObjectFlags(f.script()->fun->getType(),
> +                                types::OBJECT_FLAG_CREATED_ARGUMENTS);
> +        JS_ASSERT(!lref.isMagic(JS_LAZY_ARGUMENTS));

Great to have this assert; its rather surprising ;-)

::: js/src/vm/Stack.h
@@ +576,5 @@
>  
>      inline uintN numActualArgs() const;
>      inline js::Value *actualArgs() const;
>      inline js::Value *actualArgsEnd() const;
> +    inline void ensureCoherentArgCount();

From IRL discussion: I think it would be somewhat cleaner to remove this member function and have the invariant:

  fp->script()->usesArguments implies fp->args.nactual is initialized

To make this true it seems like the only change needed is to add the following to initJitFrameLatePrologue:

  if (!(flags_ & (OVERFLOW_ARGS | UNDERFLOW_ARGS)))
      args.nactual = fun()->nargs;

so that it matches the actual late prologue of generatePrologue.

Along the same lines, it seems the "fp->scopeChain()" call in EnterMethodJIT can be dropped by adding a scopeChain assignment to initCallFrameLatePrologue.

Comment 29

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

Comment on attachment 532851 [details] [diff] [review]
script related changes

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

Sorry for the delay!

::: js/src/jscompartment.cpp
@@ -594,5 @@
> -{
> -    if (BackEdgeMap::Ptr p = backEdgeTable.lookupWithDefault(pc, 0))
> -        return ++p->value;
> -    return 1;  /* oom not reported by backEdgeTable, so ignore. */
> -}

What happened to the backedge table? Is there a new heuristic to delay compilation?

::: js/src/jsemit.cpp
@@ +1420,5 @@
>  }
>  
> +static inline bool
> +MaybeEmitTypeSet(JSContext *cx, JSCodeGenerator *cg, JSOp op)
> +{

Could you write something brief on when JOF_TYPESET needs to be attached to an opcode and what it means?

::: js/src/jsinterp.cpp
@@ +5200,5 @@
> +     * a use of the variable.
> +     */
> +     uint32 slot = GET_SLOTNO(regs.pc);
> +     JS_ASSERT(slot < script->nslots);
> +    PUSH_COPY_SKIP_CHECK(regs.fp()->slots()[slot]);

missing a space

::: js/src/jsiter.cpp
@@ +1252,5 @@
> +
> +            jsbytecode *yieldpc = gen->regs.pc - JSOP_YIELD_LENGTH;
> +            JS_ASSERT(JSOp(*yieldpc) == JSOP_YIELD);
> +
> +            script->typeMonitorUnknown(cx, yieldpc);

It would be nice to have documentation on where these calls have to be placed and what exactly they do.

::: js/src/jsopcode.cpp
@@ +2143,5 @@
>                  else if (op == JSOP_GETELEM2)
>                      saveop = JSOP_GETELEM;
>              }
> +            // :FIXME: does later code depend on this assert? It can trip with JOF_TYPECHECK opcodes.
> +            //LOCAL_ASSERT(js_CodeSpec[saveop].length == oplen ||

:FIXME:

::: js/src/jsscript.h
@@ +612,5 @@
> +    inline void typeSetArgument(JSContext *cx, unsigned arg, js::types::jstype type);
> +    inline void typeSetArgument(JSContext *cx, unsigned arg, const js::Value &value);
> +    inline void typeSetArgument(JSContext *cx, unsigned arg, js::types::ClonedTypeSet *types);
> +    inline void typeSetUpvar(JSContext *cx, unsigned upvar, const js::Value &value);
> +

Could all of this stuff be in a separate, TIScript hanging off JSScript instead?

::: js/src/jsscriptinlines.h
@@ +102,5 @@
> +{
> +    if (!lastBinding) {
> +        lastBinding = EmptyShape::create(cx, &js_CallClass);
> +        if (!lastBinding) {
> +            js_ReportOutOfMemory(cx);

EmptyShape::create reports an error so just propagate false.

Comment 30

[Brian Hackett [Laid off!]]

(In reply to comment #29)
> ::: js/src/jscompartment.cpp
> @@ -594,5 @@
> > -{
> > -    if (BackEdgeMap::Ptr p = backEdgeTable.lookupWithDefault(pc, 0))
> > -        return ++p->value;
> > -    return 1;  /* oom not reported by backEdgeTable, so ignore. */
> > -}
> 
> What happened to the backedge table? Is there a new heuristic to delay
> compilation?

With inference on we just use the use count on the script (renamed from callCount), so we can reset the count after recompiling.  There isn't a way to reset entries in the backedge table (they even stick around after the script is destroyed).

I added the backedge table back a couple days ago (rev 8a0b550c10eb), for use when inference is disabled.  Removing it affected when we decided to trace and had a chaotic performance effect --- SS regressed a few ms, and v8-crypto slowed down a lot.

> ::: js/src/jsscript.h
> @@ +612,5 @@
> > +    inline void typeSetArgument(JSContext *cx, unsigned arg, js::types::jstype type);
> > +    inline void typeSetArgument(JSContext *cx, unsigned arg, const js::Value &value);
> > +    inline void typeSetArgument(JSContext *cx, unsigned arg, js::types::ClonedTypeSet *types);
> > +    inline void typeSetUpvar(JSContext *cx, unsigned upvar, const js::Value &value);
> > +
> 
> Could all of this stuff be in a separate, TIScript hanging off JSScript
> instead?

It could, but what would the purpose be?  We would save a few words per script with inference disabled, use a few more with inference enabled, and have another structure to keep track of.

Comment 31

[Brian Hackett [Laid off!]]

Attachment: script .h changes

Move most type-related methods out of JSScript and into an embedded TypeScript field.  This doesn't move all the analysis/inference-related methods, to distinguish between information that is discarded after each GC (ScriptAnalysis) and information that is retained until the script is destroyed (TypeScript).

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 537004 [details] [diff] [review]
GC mark changes

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

Looks good.

::: js/src/jsgcmark.cpp
@@ +866,5 @@
> +    /*
> +     * Only mark types if the Mark/Sweep GC is running; the bit won't be cleared
> +     * by the cycle collector.
> +     */
> +    if (trc->context->runtime->gcMarkAndSweep)

Could you change this to IS_GC_MARKING_TRACER(trc)?

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 532854 [details] [diff] [review]
object changes

r+. Please do fix the style nits and comments, though.

Comment 34

[Brian Hackett [Laid off!]]

(In reply to comment #33)
> Comment on attachment 532854 [details] [diff] [review] [review]
> object changes
> 
> r+. Please do fix the style nits and comments, though.

Most of the nits should be fixed on JM tip now, though I still need to do some of the tracer function renaming (will do that before merging).