694561 - Remove JSObject::flags

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Removing flags will cut the size of JSObject to its target size, 4 words.  A fair number of flags have already been removed by other object shrinking stuff.  The remainder are doable in three steps:

- Move numFixedSlots to Shape.
- Encode SINGLETON_TYPE / LAZY_TYPE flags in type information.
- Move all other flags to BaseShape.  Bug 638316 added a Shape::replaceLastProperty which allows objects to be reshaped for changes in the BaseShape, without first converting to dictionary mode.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: part 1

Move fixed slot count to Shape.

https://hg.mozilla.org/projects/jaegermonkey/rev/ba69a907d9a4

Comment 2

[Brian Hackett [Laid off!]]

Attachment: part 2

Move rarely set object flags to BaseShape.

https://hg.mozilla.org/projects/jaegermonkey/rev/8f3d52b0fc52

Comment 3

[Brian Hackett [Laid off!]]

Attachment: part 3

Move singleton type state into TypeObject, remove flags and get object to four words.

https://hg.mozilla.org/projects/jaegermonkey/rev/d72fbcc87e6b

The object shrinking stuff still needs more tuning for benchmarks --- earley-boyer and deltablue don't change much wrt m-c and splay crashes, but there is a good speedup on microbenchmarks.

function foo() {
  for (var i = 0; i < 10000000; i++)
    var x = {f:0,g:1};
}
foo();

Shell times (THREADSAFE builds will be faster due to background sweeping):

js -m -n (m-c): 500
js -m -n (JM):  322
d8:             317

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 567139 [details] [diff] [review]
part 1

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

::: js/src/jsobj.cpp
@@ +4556,5 @@
> +    if (clasp->flags & JSCLASS_HAS_PRIVATE) {
> +        JS_ASSERT(slots > 0);
> +        slots--;
> +    }
> +    return slots;

I think you can replace the whole body with:

  return GetGCKindSlots(getAllocKind(), clasp);

::: js/src/jsregexpinlines.h
@@ +748,5 @@
>       * will already have the relevant properties, at the relevant locations.
>       */
>      if (nativeEmpty()) {
>          const js::Shape *shape = js::BaseShape::lookupInitialShape(cx, getClass(), getParent(),
> +                                                                   js::gc::FINALIZE_OBJECT8, lastProperty());

Could there be a RegExpObject::FINALIZE_KIND or something we use instead of FINALIZE_OBJECT8 everywhere?  It is not unlikely someone will want to change one of these numbers in the future and it would be nice if they could in one place.  There are bunch of cases other than this; can you grep this patch for FINALIZE_ and factor each into a new constant for the class?

::: js/src/jsscope.cpp
@@ +1232,5 @@
> +    }
> +
> +    Shape *&shape = entry->shapes->get(kind);
> +
> +    if (shape)

can nix \n between decl and test

::: js/src/jsscope.h
@@ +485,5 @@
> +    enum {
> +        /* Number of fixed slots in objects with this shape. */
> +        FIXED_SLOTS_MAX        = 0x1f,
> +        FIXED_SLOTS_SHIFT      = 27,
> +        FIXED_SLOTS_MASK       = FIXED_SLOTS_MAX << FIXED_SLOTS_SHIFT,

There is a bit of a portability danger using enums when ~ is used on the enumerators (since the underlying integer type is not defined).  You can use JS_ENUM_HEADER/JS_ENUM_FOOTER to specify the underlying type (see JSValueType in jsval.h).

@@ -494,5 @@
> -     * Index in object slots for shapes which hasSlot(). For !hasSlot() shapes
> -     * in the property tree with a parent, stores the parent's slot (which may
> -     * be invalid), and invalid for all other shapes.
> -     */
> -    uint32              slot_:29;

Need to update "slot_"-containing comment below.

@@ +912,5 @@
> +    }
> +};
> +
> +/* Heap-allocated array of shapes for each object allocation size. */
> +class ShapeKindArray

It seems the JSCompartment::BaseShape use can hold non-empty shapes but the TypeObject::emptyShapes only holds empty shapes.  Could you make this a template <class ShapeType> and enforce this distinction statically?  That would also remove the static_cast in TypeObject::getEmptyShape.

@@ +930,5 @@
> +
> +    Shape *&getIndex(size_t i) {
> +        JS_ASSERT(i < SHAPE_COUNT);
> +        return shapes[i];
> +    }

Instead of returning references to private data (which makes it hard to discern invariants like, e.g., is a non-null shapes[i] element ever updated?), could you have get/set members?  This would also avoid some lexically non-obvious uses of mutable references in the callers of get() and getIndex().

::: js/src/jsscopeinlines.h
@@ +234,1 @@
>                                  other->flags, other->shortid_);

Can you assert numFixedSlots() == other->numFixedSlots()?

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 567174 [details] [diff] [review]
part 2

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

::: js/src/jsobj.h
@@ +782,5 @@
>          fixedSlots()[slot] = value;
>      }
>  
>      /* Defined in jsscopeinlines.h to avoid including implementation dependencies here. */
> +    inline bool updateFlags(JSContext *cx, jsid id, bool isDefinitelyAtom = false);

This name has been bugging me a bit because it's so general sounding.  Perhaps updateFlagsIfElementId?

::: js/src/jsscope.cpp
@@ +810,5 @@
>           * We are done updating shape and the last property. Now we may need to
>           * update flags. In the last non-dictionary property case in the else
>           * clause just below, getChildProperty handles this for us. First update
>           * flags.
>           */

The comment seems out of date or at least confusing.

::: js/src/jsscope.h
@@ +934,5 @@
>  {
>      EmptyShape(BaseShape *base, uint32 nfixed);
>  
>      static EmptyShape *create(JSContext *cx, js::Class *clasp, JSObject *parent, uint32 nfixed) {
> +        BaseShape lookup(clasp, parent, 0);

Could you make the objectFlags parameter default and drop the 0 here (this avoids the unadorned 0 which requires looking at the method decl to see what it means).

::: js/src/jsscopeinlines.h
@@ +133,5 @@
>  {
>      JS_ASSERT(nativeEmpty());
>      JS_ASSERT(getAllocKind() == gc::FINALIZE_OBJECT2);
>  
> +    if (isDelegate()) {

This gnarliness is growing and is in two places; could you factor it out, say, into BaseShape::getInitialShapeForBuiltin?  In particular, the lookupInitialShape(..., lastProperty()) + assignInitialShape + insertInitialShape dance is only used for StringObject/RegExpObject so perhaps you could split lookupInitialShape into:

  lookupInitialShape : ends early by returning entry->shapes->get(kind) 

  getInitialShape : does what lookupInitialShape does now when initial == NULL (calls lookupInitialShape)

and have BaseShape::getInitialShapeForBuiltin call lookupInitialShape directly and then do the assignInitialShape (passed and called via memfun ptr) and table update.

I just noticed the current BaseShape::lookup(InitialShape) names should really be prfixed with 'get' since 'lookup' means "try to find it" but not "and create it if it isn't there" (cf. HashMap::lookup, Shape::getChildBinding, JSObject::getChildProperty).  Could you s/lookup/get/?

Lastly, could you move StringObject::assignInitialShape to vm/StringObject.cpp.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 567174 [details] [diff] [review]
part 2

Great job on all this!