584917 - TM: Make JSObject flexible length

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: Patch for jsobj.h

It would be good if JSObjects could have a variable number of fslots, depending on their anticipated use.  On SS, my machine spends about 10ms (maybe 12ms on AWFY) allocating, filling in and freeing 7 element arrays (90% of all arrays), and allocating the slots for these inline would be much faster than the existing malloc, and faster and simpler than using a bump allocator on the side (bug 581586).  Plus an unknown amount of overhead in other objects with more fslots than needed, objects with dynamically allocated slots, and branching/arithmetic needed to look up a slot.

Attached is a patch for jsobj.h allowing objects to have any number of fslots.  Getting this to work with GC arenas would require having separate arenas for each length of fslots, e.g. 0, 2, 4, 8.  Hard to say what this will do to the average size of allocated objects --- there are two more words in the base case, for capacity and the private value, which aren't used by all objects.  The private value could move back to JSSLOT_PRIVATE, but was taken out so that it could store the length of dense arrays, and dense array accesses behave exactly the same as slot accesses.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 463376 [details] [diff] [review]
Patch for jsobj.h

What is the win in removing JSSLOT_PRIVATE?

This patch must not be complete, since JSSLOT_START and JSSLOT_FREE are used in .cpp files but removed by the patch to jsobj.h here.

/be

Comment 2

[Brian Hackett [Laid off!]]

This patch is just showing how JSObject would change, not by any stretch complete.  Making JSSLOT_PRIVATE a separate field helps with consistency between dense arrays and other objects in accessing slots, obj->getDenseArrayElement(i) == obj->getSlot(i) == obj->dslots[i].  The array needs to store its length somewhere, and private seems the only place, but if that private is in a slot then the above equalities can't work.  Also, for classes with JSCLASS_HAS_PRIVATE this saves a word on 32 bit platforms.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch working for SS

This benchmark works for x64 on SS (haven't tried anything else).  Still need to work on using inline slots for other objects (e.g. currently mallocs for every object made by access-binary-trees), but once that's done should be able to avoid allocation for the vast majority of objects in SS, including not just tiny objects but also fairly small objects with predictable lengths like the initializers in date-format-tofte (which run 1000x each).

For x64 TM this benchmark goes from 112ms to 70ms (JSC is 68ms):

function foo() {
  for (var i = 0; i < 500000; i++) {
    var a = Array();
    a[0] = 0;
  }
}
foo();

One perf downside to this is that all accesses to slots are through the dynamic slots pointer, not direct to fslots.  By getting the scope to ensure that two objects with the same shape have the same number of inline slots, the tracing/method JITs could access the fslots directly.  I measure 1.2m direct accesses (i.e. from get/setprop and friends) to slots 0-2 on SS, and 65m on V8.  From microbenchmarks (probably not too far off in this case), this gives a slowdown for me of .8ms on SS, and 50ms on V8.

Incidentally, there are 2.2m accesses on SS to the first 8 slots, and 120m on V8.  Ideally we could guess inline lengths right so that most of these accesses
are to fslots, without bloating the size of all objects.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch reducing allocations

This patch reduces the total number of slot allocations to about 8k, mostly in tofte and tagcloud.  Saves 6ms for me if I run the tests individually, 4ms in the sunspider harness.

Aside: the sunspider harness runs all tests in the same process, and GCs after each one; this cuts faulting way down, and saves 25ms for me over running all tests in one process without GCs (which I think is how the browser currently does things).  Is there a plan to finagle the browser to GC between adjacent tests?

Comment 5

[Brendan Eich [:brendan]]

See bug 547327 (on Gregor's list atm) for the predict-object-size-from-object-allocation-site discussion.

It would be great to get both of these bugs patched together so we could see how well we do with optimized fslots allocations.

There's no reason the tracer should go through dslots for fslots even now. That is a pre-requisite for landing, and it should be easy. Please feel free to ping me, gal, jorendorff, or anyone who has worked on tracing shape-guarded slot accesses.

/be

Comment 6

[Brendan Eich [:brendan]]

Once we have bug 558451 fixed, and this bug and bug 547327 patched, we can try unboxed types determined by shapes, with fallback to js::Value as needed. This could be a big win.

/be

Comment 7

[Brian Hackett [Laid off!]]

Few things:

Yes, getting direct fslots accesses shouldn't be too hard.  Vis a vis the current JSScopes, this should just require all objects rooted at the same JSEmptyScope to have the same number of fslots.  Downside: all accesses must go through dslots for objects which have dynamically allocated slots; the fslots won't be used anymore in such cases.  Upside: for objects with more fslots more accesses will be direct; from comment 3, an additional 1m accesses in SS can be done directly if enough slots are preallocated, mostly in access-nbody.

From measurements I did when working on this patch, the number of fslots used *does* make a significant difference in allocation overhead for object-heavy code; using 4 fslots where appropriate (access-binary-trees, crypto-aes) instead of 8 probably adds up to 3+ ms over sunspider, so the approach in bug 547327 is definitely worth trying.

I've thought about using unboxed values for loop variables, basically a lightweight inference/analysis that can lift guards out, e.g. generate code without type tests for i/n here but recompile if i++ ever overflows or the incoming n is not an integer.

for (i = 0; i < n; i++) ...

This wouldn't I think be too hard (self contained, doable for 4.0), maybe extensible to object shapes as you propose.  I haven't pursued this (yet) because earlier experiments by me and dmandelin seemed to show we need real regalloc for unboxed values to be a significant speedup (but if you have both, the speedup is huge), and VM stuff still seems the most important to work on.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Few things:
> 
> Yes, getting direct fslots accesses shouldn't be too hard.  Vis a vis the
> current JSScopes, this should just require all objects rooted at the same
> JSEmptyScope to have the same number of fslots. Downside: all accesses must go
> through dslots for objects which have dynamically allocated slots;

Yes, this is an important point.

Right now we have empty shape per prototype where class of proto and new object are the same. But certain objects have dynamic number of slots preallocated per instance, not per class -- see NewArguments, NewCallObject, e.g.

This means the first ad-hoc ("expando", user-set) slot is not constant for the given class. Thus the property tree has slot as well as id, attrs, etc. -- slot cannot be predicted from parent (empty as basis case) shape.

Bug 558451 is not going to fix this, as getting rid of JSScope allocations is the goal and that win is overdue. But in followup bugs I would like to make the first property's slot class-invariant.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> This means the first ad-hoc ("expando", user-set) slot is not constant for the
> given class. Thus the property tree has slot as well as id, attrs, etc.

Not only has slot, a property tree node's identity (hash, match) depends on slot as well as other members. If first property slot were class-invariant the slot could be left out of this identity.

/be

Comment 10

[Brian Hackett [Laid off!]]

Attachment: updated patch

Patch fixing all tests and trace-test failures, except basic/testHoleInDenseArray.js has three side exits instead of two.  How do I debug this?

Comment 11

[Brendan Eich [:brendan]]

I usually set env TMFLAGS full in gdb and run piped to grep leaving to see where the exits come from in terms of source line and bytecode offset. Do the same with an unpatched shell.

/be

Comment 12

[Brian Hackett [Laid off!]]

OK, this failure (discrepancy?) looks benign to me.  Here is the meat of the test:

function f(i) {
    if (i > 4) /* side exit when arr[i] changes from bool to undefined (via a hole) */
        assertEq(s, undefined);
    else
        assertEq(s, false);
    return 1;
}

var arr = [ false, false, false, false, false, , , , ];

for (var i = 0; i < 10; ++i) {
    (s = arr[i]) + f(i);
}

The first side exit (both patched and unpatched) is on reading the first hole, when i == 5.  The trace is retriggered when i == 7, and here is where things differ.

In the unpatched version, the array's capacity is 7 (so index is out of bounds), and a trace is emitted for getelem which checks that the index is out of bounds (then produces undefined).  This holds for the remainder of the array.

In the patched version, the array's capacity is 8 (since there is no dslots[-1] anymore, capacities are aligned to powers of 2), and a trace is emitted which checks that the index is in bounds.  This guard trips on the next iteration.

Should I just change this test to be less sensitive to the capacity of the array?

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> Should I just change this test to be less sensitive to the capacity of the
> array?

Yes, that sounds like the best fix -- these tests are sometimes just-so stories but we can fix them to be less dependent on tunable parameters of the VM and JITs.

/be

Comment 14

[Brian Hackett [Laid off!]]

Attachment: patch with direct fslots accesses

This patch modifies JSEmptyScope to keep track of the number of fixed slots in the object, so that objects with the same properties but different numbers of fixed slots have different shapes, and the fslots are accessed directly in shape-guarded TM JIT code.  I get the same performance as stock TM in loops accessing object fslots.  Passes tests and trace-tests, in x86 and x64.  Haven't tested anything wrt the browser.  Is this ready for review?

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)

This will collide badly with the patch queue for bug 558451, but more important than that: JSEmptyScope is already specialized to a given JSCLASS_FREE(clasp).

What objects have different numbers of fixed slots per instance (not per class) other than the arguments, call, etc. special cases mentioned in comment 8?

/be

Comment 16

[Brian Hackett [Laid off!]]

The inline slot capacity should be orthogonal to the per-instance reserved slot count used for arguments etc. objects (though it should be the case that either the inline slot capacity is zero or is >= the reserved count).

With this patch, any class can have different numbers of inline slots per instance, though there are only a few possible such numbers (0, 2, 4, 8 in the patch, extremely tunable).  This is most important for Array and Object, where reserved slots are always the same but different inline capacities can be used based on the expected number of properties the script will add.

Comment 17

[Brendan Eich [:brendan]]

I talked to Brian.

We can elaborate the new JSEmptyScope (want to rename these things, but later), after bug 558451 is fixed. So I'll work on getting that patch queue reviewable, reviewed, and landed, and bhackett will then rebase on it.

Thanks,

/be

Comment 18

[Nicholas Nethercote [inactive]]

I like the direction this bug is headed, except for one thing.

We're heading in the direction of quasi sub-classes for specific object kinds -- DenseArrayObject, RegExpObject, DateObject, etc.  See bug 566789 for an example.  As part of that, a call like this:

  obj->getDenseArrayElement(i)

will become this:

  obj->asDenseArray()->getElement(i)

asDenseArray() contains an assertion that the object really is a DenseArray.

The attached patch converts all these getDenseArrayElement(i) calls to getSlot(i) calls, which is a backward step.  So I'd like to see them converted back.  For the moment, getDenseArrayElement() can just assert the object is a DenseArray and then call onto getSlot().  Make sense?  Thanks.

Comment 19

[Brian Hackett [Laid off!]]

Attachment: rebased patch

This rebases the flexible length objects on current TM.  This saves me 7.5ms on SS (so 10ms or so on AWFY), mainly 3d-cube and crypto-aes (both make lots of short arrays).  This also restores the dense array JSObject methods, per comment 18.

Comment 20

[Brian Hackett [Laid off!]]

Forgot: this passes tests and trace-tests, haven't tested with try server.  Who should review this?

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 474928 [details] [diff] [review]
rebased patch

>     /* Begin with the length property to share more of the property tree. */
>     if (!addProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.lengthAtom),
>                      array_length_getter, array_length_setter,
>-                     JSSLOT_ARRAY_LENGTH, JSPROP_PERMANENT | JSPROP_SHARED, 0, 0)) {
>+                     SHAPE_INVALID_SLOT, JSPROP_PERMANENT | JSPROP_SHARED, 0, 0)) {

Comment that the getter and setter update the object's private data member.

>         if (getDenseArrayElement(i).isMagic(JS_ARRAY_HOLE)) {
>-            setDenseArrayElement(i, UndefinedValue());
>+            setSlot(i, UndefinedValue());

Comment 18 still wanted setDenseArrayElement here, no leaky setSlot abstraction (however temporary), right?.

>-    memcpy(obj->getDenseArrayElements(), vector, length * sizeof(Value));
>+    memcpy(obj->addressOfDenseArrayElement(0), vector, length * sizeof(Value));
. . .
>-            Value *elems = obj->getDenseArrayElements();
>+            Value *elems = obj->addressOfDenseArrayElement(0);
. . .
>-                Value *elems = obj->getDenseArrayElements();
>+                Value *elems = obj->addressOfDenseArrayElement(0);

Worth keeping getDenseArrayElements() to minimize the diff and slightly shorten names and arglists, I think.

>-            if (!InitArrayObject(cx, obj2, count, obj->getDenseArrayElements() + begin))
>+            if (!InitArrayObject(cx, obj2, count, obj->addressOfDenseArrayElement(0) + begin))

Ditto.

>-            Value *arraybeg = obj->getDenseArrayElements();
>+            Value *arraybeg = obj->addressOfDenseArrayElement(0);

Etc. (last note, even though more below).

>-    closure->initSharingEmptyShape(&js_FunctionClass, proto, parent, fun, cx);
>+    if (!closure->initSharingEmptyShape(cx, &js_FunctionClass, proto, parent,
>+                                        fun, FINALIZE_OBJECT2))
>+        return NULL;

Nit: always brace if condition or then or else is multiline.

> JS_GetObjectTotalSize(JSContext *cx, JSObject *obj)
> {
>     size_t nbytes = (obj->isFunction() && obj->getPrivate() == obj)
>                     ? sizeof(JSFunction)

Later you test !obj->getPrivate() if obj->isFunction() to decide that obj is the compiler-created function prototype ("exemplar") of size sizeof(JSFunction) not sizeof(JSObject). Here you keep the obj->getPrivate() == obj test. Both can't be valid.

>-    uintN base = JSSLOT_FREE(&js_BlockClass);
>+    uintN base = JSCLASS_RESERVED_SLOTS(&js_BlockClass);

Keep JSSLOT_FREE to minimize patch and abstract slightly over loss of JSSLOT_PRIVATE. Or does it hurt? It seems to help, including reducing length of expressions such as the RHS of = here.

> NewCallObject(JSContext *cx, JSFunction *fun, JSObject &scopeChain, JSObject &callee)
> {
>-    JSObject *callobj = js_NewGCObject(cx);
>+    size_t vars = fun->countArgsAndVars();
>+    size_t slots = JSObject::CALL_RESERVED_SLOTS + vars;
>+    JSFinalizeGCThingKind kind = js_GetGCObjectKind(slots);
>+
>+    JSObject *callobj = js_NewGCObject(cx, kind);
>     if (!callobj)
>         return NULL;
> 
>-    callobj->init(&js_CallClass, NULL, &scopeChain, NULL, cx);
>+    /* Init immediately to avoid GC seeing a half-init'ed object. */
>+    size_t fixed = js_GetGCKindSlots(kind);
>+    callobj->init(cx, &js_CallClass, NULL, &scopeChain, NULL, fixed, false);
>     callobj->setMap(fun->u.i.names);
> 
>     /* This must come after callobj->lastProp has been set. */
>     if (!callobj->ensureInstanceReservedSlots(cx, fun->countArgsAndVars()))

Use vars local instead of repeating fun->countArgsAndVars() here.

> NewDeclEnvObject(JSContext *cx, JSStackFrame *fp)
> {
>-    JSObject *envobj = js_NewGCObject(cx);
>+    JSObject *envobj = js_NewGCObject(cx, FINALIZE_OBJECT2);
>     if (!envobj)
>         return NULL;
> 
>-    envobj->init(&js_DeclEnvClass, NULL, &fp->scopeChain(), fp, cx);
>+    envobj->init(cx, &js_DeclEnvClass, NULL, &fp->scopeChain(), fp, 2, false);

It's unfortunate we have to couple js_NewGCObject's second arg to the clasp arg of JSObject::init, specifically its JSCLASS_RESERVED_SLOTS(clasp) result.

. . .

Nice class Call slots copying changes!

> struct JSFunction : public JSObject
> {
>+    /* Space for JSObject inline slots */
>+    js::Value data_padding[FUN_CLASS_RESERVED_SLOTS];
>+
>     uint16          nargs;        /* maximum number of specified arguments,

Nit: indent data_padding like other members such as nargs.

>+JSFinalizeGCThingKind js_GCObjectSlotsToThingKind[] = {
>+    FINALIZE_OBJECT0, FINALIZE_OBJECT2, FINALIZE_OBJECT2, FINALIZE_OBJECT4,
>+    FINALIZE_OBJECT4, FINALIZE_OBJECT8, FINALIZE_OBJECT8, FINALIZE_OBJECT8,
>+    FINALIZE_OBJECT8, FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0,
>+    FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0,
>+    FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0,
>+    FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0,
>+    FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0,
>+    FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0, FINALIZE_OBJECT0
>+};

. . .

>-        JS_ASSERT(IsFinalizableStringKind(thingKind));
>+        JS_ASSERT(IsFinalizableStringKind(JSFinalizeGCThingKind(thingKind)));

This recurs -- can you use C++ types and conversion operators to avoid mismatching thingKind and JSFinalizeGCThingKind's return value?

>+js_GetGCObjectKind(size_t numSlots)
>+{
>+    /* This array/capacity needs to be in sync. */
>+    const size_t capacity = 32;

Lose the comment and use JS_ARRAY_LENGTH(js_GCObjectSlotsToThingKind) instead.

>+    extern JSFinalizeGCThingKind js_GCObjectSlotsToThingKind[];
>+
>+    /* TODO: measure efficiency of dereference vs. unpredictable branching. */
>+    if (JS_UNLIKELY(numSlots > capacity))
>+        return FINALIZE_OBJECT0;

Policy is to avoid JS_{,UN}LIKELY without profile data showing it wins. If you want to do that, file a bug and cite with with "FIXME bug NNNNNNN" instead of "TODO".

>+    return js_GCObjectSlotsToThingKind[numSlots];
>+}
>+
>+/* Get the number of fixed slots and initial capacity associated with a kind. */
>+static inline size_t
>+js_GetGCKindSlots(JSFinalizeGCThingKind thingKind)
>+{
>+    extern size_t js_GCObjectThingKindToSlots[];
>+
>+    /* TODO: measure efficiency of dereference vs. unpredictable branching. */
>+    JS_ASSERT(thingKind >= FINALIZE_OBJECT0 && thingKind <= FINALIZE_OBJECT_LAST);
>+    return js_GCObjectThingKindToSlots[thingKind - FINALIZE_OBJECT0];

Similar comments re: TODO.

> BEGIN_CASE(JSOP_NEWINIT)
> {
>     jsint i = GET_INT8(regs.pc);
>     JS_ASSERT(i == JSProto_Array || i == JSProto_Object);
>     JSObject *obj;
>+
>+    /*
>+     * TODO: Tailor the new object size to the size of the initializer.

FIXME bug citation or it won't happen :-P.

>+     * Except in the empty case, it is unlikely additional properties will be added.

You must mean "likely"? Not sure, but this comment looks wrong.

> NewObjectWithClassProto(JSContext *cx, Class *clasp, JSObject *proto,
>-                        const Value &privateSlotValue)
>+                        /*JSFinalizeGCThingKind*/ unsigned _kind)

Trailing _ if you must, but why is it needed here? Oh, thingKind vs theOtherThingKind. I hope some spanky C++ value types can help.

>+    if (newcap > MAX_DSLOTS_LENGTH32) {

NSLOTS_LIMIT is the new bound -- sorry I didn't fix this but since you are touching it, can you do the deed?

Out of time, but looks pretty good. I have some more comments, but if you want to act on these I'll resume in a new patch. Thanks again,

/be

Comment 22

[Brendan Eich [:brendan]]

> This recurs -- can you use C++ types and conversion operators to avoid
> mismatching thingKind and JSFinalizeGCThingKind's return value?

I didn't spot any such bugs, just noticed overlong expressions and the _kind vs. kind stuff, meant by the above that we could avoid "kind confusion" bugs with some C++ nominal typing and conversions. Not sure it won't ding perf, though. Worth a shot?

/be

Comment 23

[Brian Hackett [Laid off!]]

Attachment: revised patch

Fixes for comment 21.

The unsigned instead of JSFinalizeGCThingKind stuff is kind of annoying, the problem is that jsobj.h needs to refer to the kinds but does not include jsgc.h, and enums can't have forward declarations.  The changes to IsFinalizableStringKind were leftovers from an attempt to fix this, now reverted.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 475214 [details] [diff] [review]
revised patch

I had a bunch of comments, all lost when minefield crashed toward the end. From memory:

* consider s/priv/private_/ or even /privateData/
* ditto data -> inlineSlots, we arguably want more descriptive member names and "priv" and "data" both connote "private data".
* s/ind/i/g -- use canonical loop variable naming
* calloc emptyShapes and create lazily, don't make all at once up front
* jsgc.h includes jsobj.h so I still think a subset kind enum in the latter works and the former supersets/renames it
* the magic 32 still bugs -- JS_STATIC_ASSERT that it equals JS_ARRAY_LENGTH()
 if you can (in jsgc.cpp, which would require making the capacity visible outside of the inline function)

Great work, can't wait for this to land.

/be

Comment 25

[Brendan Eich [:brendan]]

Again, FIXME bug NNNNNN or it (didn't / won't) happen :-/. Down with XXX and TODO!

 * TODO: Need heuristics for guessing likely object sizes.

This should say FIXME bug 547327

         * TODO: Maybe better to do this initialization lazily.  This will

Use cx->calloc and create empty shapes on demand and this goes away. Don't see a good reason not to be fully lazy.

/be

Comment 26

[Brian Hackett [Laid off!]]

Attachment: revised patch

This fixes a performance regression on v8-splay, whose GeneratePayloadTree exhibited pathological behavior wrt the last patch --- the patch kept allocating 8 element arrays for initializers with 10 slots, and 8 element objects for initializers with two slots.  This modifies NEWINIT to track the size as well as the length of initializers, and tailor the size of new objects to those initializers.  For splay on x86 and x64 the total memory used is similar with or without this patch, though ~25MB are now in the GC heap instead of the malloc heap.

V8 performance is a wash, SS still improves by 7ms.  On SS there is a 2.5ms regression on date-format-tofte, which may just be an artifact of the way the SS harness runs things (GCs between every regression).  If I don't figure out or fix what's going before landing, will file a followup bug for that.

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 476357 [details] [diff] [review]
revised patch

> +     * case where length is <= capacity the clone and original array will have

Nit: comma after "capacity" (matches version of essentially the same text in prior comment).

> +     * that two objects with the same shape use their fixed slots in the same way.

Nit: overlong line.

More seriously, is JSObject::swap using generateOwnShape and allocSlots unnecessarily? Proposal:

1. If this and other both have only fixedSlots, do not generateOwnShape or allocSlots.
2. Else if the one that is not using fixedSlots could fit its slots in fixedSlots, switch it back and generateOwnShape.
3. Else generateOwnShape and allocSlots only for the one that is using fixedSlots.

How can JSObject::swap use tmp if this or other is FINALIZE_OBJECT12 or FINALIZE_OBJECT16 (iirc bigger than JSFunction)?

JSObject::swap is a sharp too, its caller could be required to allocate a same FINALIZE_OBJECT* class thing for the proxy (e.g.) as for the native that is going to become the proxy. Indeed this seems like it would simplify everything, including the issue of whether both objects are using fixedSlots or not. We should not over-generalize swap.

clearSlotsArray frees -- is it misnamed?

JSOP_NEWINIT should be JOF_UINT24 or even JOF_UINT32 and interp/jits crack the bits?

Make fixedSlots[] not [2], get rid of JSOBJECT_SIZE

SLOTS_TO_THING_KIND_CAPACITY -- s/CAPACITY/LIMIT

Any data backing the default of GuessObjectGCKind to FINALIZE_OBJECT8 if numSlots == 0? Could this be affecting tofte?

/be

Comment 28

[Brendan Eich [:brendan]]

> JSObject::swap is a sharp too

tool, of course ;-).

/be

Comment 29

[Brian Hackett [Laid off!]]

(In reply to comment #27)
> Make fixedSlots[] not [2], get rid of JSOBJECT_SIZE

I would love to do this, but newer versions of gcc and msvc reject it using the compiler switches we pass.  Apparently empty arrays (whether [0] or []) are not ISO C++, despite the fact that almost all systems C/C++ code uses them in some way.

Comment 30

[Brendan Eich [:brendan]]

+    js::Value   *slots;                     /* dynamically allocated slots,
+                                             * or pointer to fixedSlots */
+    js::Value   fixedSlots[2];              /* inline slots for object. count is not fixed,
+                                             * may be less or greater than two */

Nit: skip the * at the start of the comment overflow lines, underhanging the * in "/*" -- prevailing style doesn't do that for "minor comments tabulated on the right of member declarations".

/be

Comment 31

[Brendan Eich [:brendan]]

+    inline void traceEmptyShapes(JSTracer *trc);

Unused, remove.

     uint32 numSlots(void) const {
-        return dslots ? dslots[-1].toPrivateUint32() : uint32(JS_INITIAL_NSLOTS);
+        return capacity;
     }

     size_t slotsAndStructSize(uint32 nslots) const;
     size_t slotsAndStructSize() const { return slotsAndStructSize(numSlots()); }
Nit: numSlots can fit on one line, just as slotsAndStructSize and other inline shorties in JSObject do.

/be

Comment 32

[Brendan Eich [:brendan]]

Brian, did you get anywhere on the date-format-tofte regression? Let me know if you want r+ on a final patch and I'll do it either way (tofte fixed or filed as a followup).

/be

Comment 33

[Brian Hackett [Laid off!]]

I'm fairly sure the tofte thing is a quirk of the sunspider harness, and how it GCs after every test (different memory fault behavior with malloc heap vs. GC heap).  If I run the sunspider harness without calls to gc(), tofte improves .5ms with this patch, and SS improves 9ms overall.  I think I've asked this before, but in the browser do we make an effort to GC after each test in SS?  The shell harness is 25ms faster for me with the calls to gc() than without, I think mainly because far fewer faults are taken.

I'll have a final patch for you tonight.

Comment 34

[Brian Hackett [Laid off!]]

Attachment: revised patch

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 476691 [details] [diff] [review]
revised patch

Looks great -- thanks!

/be

Comment 36

[Boris Zbarsky [:bzbarsky]]

> but in the browser do we make an effort to GC after each test in SS? 

"maybe".  There are pageloads involved which should at least try to trigger gc, but avoid it if the loading is continuous or something.... until 10 seconds have passed, when something else might happen.

Comment 37

[Luke Wagner [:luke]]

P.S., bug 558861 also seems about to land which seriously refactors jsgc and conflicts with this patch.

Comment 38

[Brian Hackett [Laid off!]]

Attachment: patch fixing tracer allocation

This fixes the tracer to follow the same heuristics as the interpreter and JM in picking fslots for new objects.  All should get a speedup on SS, V8 is wonky because moving allocated data from the malloc heap into the GC heap changes when GCs occur.  Using the current mechanisms, splay and earley-boyer both incur an extra GC (which costs 75ms+ just for splay), and raytrace GCs at an earlier, worse time (bigger live heap).  If I change GC_HEAP_GROWTH_FACTOR from 1.5 to 1.8 (which this patch doesn't do), I get the same number of GCs before and after and all the monkeys improve on V8.

This will conflict with 558861 but not horribly; I will wait for 558861 to land.

Comment 39

[Brendan Eich [:brendan]]

Comment on attachment 477132 [details] [diff] [review]
patch fixing tracer allocation

Could lose the single-use kind local variables in js_InitializerArray, etc. We generally avoid, unless the consuming expression would be too complicated or there is a likely debugger need to see the value in a local, not just passed into a callee. Up to you.

/be

Comment 40

[Brendan Eich [:brendan]]

(In reply to comment #27)
> Any data backing the default of GuessObjectGCKind to FINALIZE_OBJECT8 if
> numSlots == 0? Could this be affecting tofte?

Didn't see an answer to this q. Any data, or intuition?

/be

Comment 41

[Nicholas Nethercote [inactive]]

I realize this is late to be nit-picking, but is there a good reason why 'fslots' and 'dslots' were renamed to 'fixedSlots' and 'slots'?  This seems like change for change's sake, and I like the old names better.

Comment 42

[Brendan Eich [:brendan]]

"slots is slots"?

I'm kind of fond of the old names too, but Brian with with slots, I think, because you can *always* dereference obj->slots to get at the object's value storage. The original renaming of fslots as "data" I shot down, so that got the spellItOutInCamelCaps(ButNotTooVerbosely) treatment.

We could go back, but does dslots really work now after all these years where it was a maybe-null?

/be

Comment 43

[Brian Hackett [Laid off!]]

(In reply to comment #40)
> (In reply to comment #27)
> > Any data backing the default of GuessObjectGCKind to FINALIZE_OBJECT8 if
> > numSlots == 0? Could this be affecting tofte?
> 
> Didn't see an answer to this q. Any data, or intuition?
> 
> /be

Oops, missed this.  This size is only used for empty initializers ([] and {}), as a guess that they will later have properties added.  For arrays in SS/V8 we know that almost all arrays will end up with at least one element, and the great majority will have <= 8.  No data for {}.  Either way these don't have much effect on the benchmarks, [] and {} aren't used much.

Comment 44

[Brian Hackett [Laid off!]]

I think using obj->dslots would be misleading because, as comment 42 points out, they're always there and not always dynamically allocated.  fixedSlots could go back to fslots, I picked the cumbersome name because it emphasizes that JSObject-external code should not be accessing this member (would be neat if more things in JSObject were private to match the treatment JSStackFrame has been getting).  Changing the names of slots and fixedSlots wouldn't affect non-JSObject:: code much, as pretty much everything goes through getSlot now; slots are used in a uniform way with this patch, no special treatment for dense arrays or call objects.

Comment 45

[Brian Hackett [Laid off!]]

(In reply to comment #43)
> Oops, missed this.  This size is only used for empty initializers ([] and {}),
> as a guess that they will later have properties added.  For arrays in SS/V8 we
> know that almost all arrays will end up with at least one element, and the
> great majority will have <= 8.  No data for {}.  Either way these don't have
> much effect on the benchmarks, [] and {} aren't used much.

Forgot: GuessObjectGCKind is also used for Array, so Array() and Array(0) will also end up with 8 fixed slots, which is a goog guess for the same reasons as [].

Comment 46

[Nicholas Nethercote [inactive]]

(In reply to comment #44)
> I think using obj->dslots would be misleading because, as comment 42 points
> out, they're always there and not always dynamically allocated.

Fair enough!  I didn't realize this.  If there were more detailed comments in struct JSObject I would have (hint hint :).

Comment 47

[Brian Hackett [Laid off!]]

Attachment: patch for posterity

This rebases onto bug 558861.  The problem after 558861 is that now Arenas are templated on specific types and treat the size of individual objects as sizeof(that type), which doesn't really work when values are flexible length.  This patch moves the template stuff out of Arena and onto the methods doing mark/finalize (so that the Cell::mark/finalize methods are still inlined and the stride during sweep is a compile time constant).

However, I now realize now a way to keep Arena<T> largely unchanged (though I want to remove the code duplication in EmptyArenaLists and JSCompartment).  Will go and put together a patch for that.

Comment 48

[Brian Hackett [Laid off!]]

Attachment: rebased patch

This rebases onto bug 558861.  Passes jstests, I'm getting a failure in trace-tests basic/testBug597736.js, which is doing a leak test.  It seems a C stack value is sticking around after the first iteration of the loop in this test and keeps both finalize observers reachable.  If I increase the loop limit (as this patch does), all but two of the observers are collected.  I don't know if this change breaks this test.

Comment 49

[Igor Bukanov]

(In reply to comment #48)
> If I increase the loop limit
> (as this patch does), all but two of the observers are collected.  I don't know
> if this change breaks this test.

Make the leak counting loop to run to 8 and change the assertion test from base < finalizeCount() to base + 4 < finalizeCount(). That should test the change more robust.

Comment 50

[Brian Hackett [Laid off!]]

Attachment: browser comparison

Comparison for SS 0.9 and V8v5 in an x86 OS X browser (10ms for SS, 75 points for V8).  In the shell, V8 also speeds up but SS only improves by 2ms for me.  Shell SS has some weird issues, including (still) the tofte regression and a new 2.5ms regression on fannkuch.  I think this is a cache line thing: fannkuch only allocates four objects, but they are arrays whose contents used to be malloc'ed and are now GC heap allocated.  Neither this nor the tofte regression show up in the browser.

Comment 51

[Brendan Eich [:brendan]]

Comment on attachment 478708 [details] [diff] [review]
rebased patch

This is fine but I think Gregor or Igor should take a look at the GC-specific changes. Trying Gregor.

/be

Comment 52

[Igor Bukanov]

Comment on attachment 478708 [details] [diff] [review]
rebased patch

> static bool
> EmitNewInit(JSContext *cx, JSCodeGenerator *cg, JSProtoKey key, JSParseNode *pn, int sharpnum)
> {
>-    if (js_Emit2(cx, cg, JSOP_NEWINIT, (jsbytecode) key) < 0)
>-        return false;
>+    /*
>+     * Watch for overflow on the initializer size.  This isn't problematic because
>+     * (a) we'll be reporting an error for the initializer shortly, and (b)
>+     * the count is only used as a hint for the interpreter and JITs, and does not
>+     * need to be correct.
>+     */
>+    uint16 count = pn->pn_count;
>+    if (count >= JS_BIT(16))
>+        count = JS_BIT(16) - 1;

This is bogus: JS_BIT(16) is 2**16 so the if condition is always false. Have you meant if (pn->pn_count >= JS_BIT(16)) ? If so rewrite count initialization using the ? operator.

        JS_ASSERT(sharpnum < 0);
>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp
>+++ b/js/src/jsfun.cpp
>@@ -178,31 +178,32 @@ js_GetArgsProperty(JSContext *cx, JSStac
> 
> static JSObject *
> NewArguments(JSContext *cx, JSObject *parent, uint32 argc, JSObject &callee)
> {
>     JSObject *proto;
>     if (!js_GetClassPrototype(cx, parent, JSProto_Object, &proto))
>         return NULL;
> 
>-    JSObject *argsobj = js_NewGCObject(cx);
>+    JS_ASSERT(gc::GetGCKindSlots(gc::FINALIZE_OBJECT2) == JSObject::ARGS_CLASS_RESERVED_SLOTS);

This should be turned into a static assert. For that add a template class like (names here should be improved):

template<unsigned gckind> struct ObjectSlotCounter {
    JS_STATIC_ASSERT(gckind <= FINALIZE_OBJECT16);

    static const size_t result = some_perhaps_long_constant_expression_on_gckind;
};

with usage like:

JS_STATIC_ASSERT(gc::ObjectSlotCounter<gc::FINALIZE_OBJECT2>::result == JSObject::ARGS_CLASS_RESERVED_SLOTS);


diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp
>+++ b/js/src/jsgc.cpp
> namespace js{
> namespace gc{
> 
>+FinalizeKind slotsToThingKind[] = {

Any static table must be const. Also consider using uint8 for the table entries for a minmal footprint.

>+    /* 0 */  FINALIZE_OBJECT0,  FINALIZE_OBJECT2,  FINALIZE_OBJECT2,  FINALIZE_OBJECT4,
>+    /* 4 */  FINALIZE_OBJECT4,  FINALIZE_OBJECT8,  FINALIZE_OBJECT8,  FINALIZE_OBJECT8,
>+    /* 8 */  FINALIZE_OBJECT8,  FINALIZE_OBJECT12, FINALIZE_OBJECT12, FINALIZE_OBJECT12,
>+    /* 12 */ FINALIZE_OBJECT12, FINALIZE_OBJECT16, FINALIZE_OBJECT16, FINALIZE_OBJECT16,
>+    /* 16 */ FINALIZE_OBJECT16, FINALIZE_OBJECT0,  FINALIZE_OBJECT0,  FINALIZE_OBJECT0,
>+    /* 20 */ FINALIZE_OBJECT0,  FINALIZE_OBJECT0,  FINALIZE_OBJECT0,  FINALIZE_OBJECT0,
>+    /* 24 */ FINALIZE_OBJECT0,  FINALIZE_OBJECT0,  FINALIZE_OBJECT0,  FINALIZE_OBJECT0,
>+    /* 28 */ FINALIZE_OBJECT0,  FINALIZE_OBJECT0,  FINALIZE_OBJECT0,  FINALIZE_OBJECT0,
>+    /* 32 */ FINALIZE_OBJECT0
>+};

I do not see why do you need FINALIZE_OBJECT0 entries after the last FINALIZE_OBJECT16.

> bool
>+RefillFinalizableFreeList(JSContext *cx, unsigned thingKind)
>+{
>+    switch (thingKind) {


Have you considered to turn RefillFinalizableFreeList into a non-template function that takes the thingSize as a parameter to avoid the switch here? The only place where the template is relevant for efficiency is the free list build code in Arena::init called via ArenaAllocate. But that can be replaced with suitably adjusted version of the pre-compartment GC MakeNewArenaFreeList, 

http://hg.mozilla.org/tracemonkey/file/7e801a9e2276/js/src/jsgc.cpp#l588

That verssion does not use any multiplications or devisions and I do not see how the compiler can take advantage of having thing size a compiler-time constant. 

It is OK to do it in a followup bug.

> template<typename T>
> static void
> FinalizeArenaList(JSCompartment *comp, JSContext *cx, unsigned thingKind)
> {
>     JS_STATIC_ASSERT(!(sizeof(T) & Cell::CellMask));
>-    ArenaList<T> *arenaList = GetFinalizableArenaList<T>(comp, thingKind);
>-    Arena<T> **ap = &arenaList->head;
>+    ArenaList *arenaList = GetFinalizableArenaList(comp, thingKind);
>+    Arena<T> **ap = (Arena<T> **) &arenaList->head;
>     Arena<T> *a = *ap;

Such casts lead to aliasing wanings. So turn ap into Arena<FreeCell>** and move the cast to the assignments to "a". Note also that it would be necessary to make ArenaHeader::next to be Arena<FreeCell> tunrning that class into a non-template.

r+ with the above addressed.

Comment 53

[Brian Hackett [Laid off!]]

Attachment: rebased patch

Will wait to land this until after the compartment stuff blocking beta 7.  I've also had to bump the GC heap limit in the shell from 128MB to 160MB, discussed on IRC with igor:

bhackett: igor_: for 584917 I've had to increase the max GC heap size in the shell from 128MB to 160MB for v8-splay, will that be ok?
[3:07pm] igor_: bhackett: what is the reason for that? To prevent an extra GC?
[3:07pm] bhackett: no, to prevent OOM
[3:07pm] bhackett: it just barely goes over on thread-safe x64 builds
[3:07pm] bhackett: the total memory usage is about the same with the patch
[3:07pm] bhackett: it's just moving ~20-30MB from malloc heap to GC heap
[3:07pm] bhackett: and we don't count malloc heap when triggering GC oom
[3:08pm] igor_: bhackett: hm, why the GC do not runs with the test? Or does the tests really need 160MB of heap?
[3:09pm] bhackett: igor_: it needs that much heap, it allocates a huge tree which stays reachable
[3:09pm] igor_: bhackett: ok then.

Comment 54

[Gregor Wagner [:gwagner]]

I tried to apply it today but I got many conflicts. 
If we have some more days I want to take a look and run some of my GC tests with it.

Comment 55

[Brian Hackett [Laid off!]]

Attachment: rebase onto f1c60a6d07ba

Comment 56

[Brendan Eich [:brendan]]

(In reply to comment #52)
> >+    uint16 count = pn->pn_count;

Sorry I missed this. It should have generated a warning -- did it?

In general locals should avoid using over-narrow int types. It can lose perf on some target architectures, and it risks chopping high bits. int and unsigned (or intN and uintN but these are old and ugly) stand ready and are known to be at least 32 bits. For anything address-related, ptrdiff_t and size_t.

/be

Comment 57

[Brian Hackett [Laid off!]]

(In reply to comment #56)
> (In reply to comment #52)
> > >+    uint16 count = pn->pn_count;
> 
> Sorry I missed this. It should have generated a warning -- did it?

Yeah, this did generate a warning, which I missed myself.  The most recent patch fixes this.  Also, the most recent path removes a redundant memcpy in js_PutCallObject.

Comment 58

[Gregor Wagner [:gwagner]]

Brian and me had a discussion on IRC about possible regressions of this patch. People like me use ({}) to create a linked list and this uses now more memory because every object gets initially 8 slots.
This is mainly for benchmark tuning. 
I think we should go over some allocation-heavy pages and see how well the benchmark-world fits the real-world in our case and if we could do better with the new object sizes (WSS and GC frequencies...).

Should this be a followup bug?

Comment 59

[Brian Hackett [Laid off!]]

I've filed bug 602268 for one of these sites.

Comment 60

[Brendan Eich [:brendan]]

(In reply to comment #58)
> Brian and me had a discussion on IRC about possible regressions of this patch.
> People like me use ({}) to create a linked list and this uses now more memory
> because every object gets initially 8 slots.
> This is mainly for benchmark tuning. 
> I think we should go over some allocation-heavy pages and see how well the
> benchmark-world fits the real-world in our case and if we could do better with
> the new object sizes (WSS and GC frequencies...).
> 
> Should this be a followup bug?

Not yet. I kept asking about that default guess, because it felt "too big" to me based on gut (experience, implicit learning, whatever). Ideally we'd have a way to measure and adapt. We want to measure dynamic frequency. Anyone study this lately? IIRC you did, Gregor, a while ago.

/be

Comment 61

[Brian Hackett [Laid off!]]

I'll change the number of inline slots for {} to 4, to match scripted new.

Comment 62

[Boris Zbarsky [:bzbarsky]]

Note that we default arrays, at least on trace, to 7 elements in the array... does that even fit in 8 slots?

Comment 63

[Boris Zbarsky [:bzbarsky]]

I mean arrays created with [].  Or is comment 62 totally irrelevant to the discussion?

Comment 64

[Brian Hackett [Laid off!]]

Currently, with this patch using [] will make an array with 8 inline slots whether on or off trace.  It does the same thing for {}, which is probably wrong.  I'm going to change that to use 4 inline slots for {}.

Comment 65

[David Mandelin [:dmandelin]]

I just noticed that there is a call to "sleep" in ReportOutOfMemory--that doesn't build on Windows.

Comment 66

[Brian Hackett [Laid off!]]

Oops, that's debug code left over from looking at the OOM on v8-splay, you can just remove it.

Comment 67

[Brian Hackett [Laid off!]]

Attachment: rebase

Comment 68

[Brian Hackett [Laid off!]]

Attachment: patch allowing swaps of different sized non-natives

This allows JSObject::swap to operate on objects with different numbers of fixed slots, provided they are non-native.  This also makes slotsToThingKind non-const --- to link right, Windows requires a const definition and GCC requires a non-const definition.  This array isn't exposed in any headers so that seems enough protection.

Comment 69

[Brendan Eich [:brendan]]

Comment on attachment 482740 [details] [diff] [review]
patch allowing swaps of different sized non-natives

>+JSObject::swap(JSContext *cx, JSObject *other)
> {
>     size_t size = GetObjectSize(this);
>+
>+    if (size != GetObjectSize(other)) {
>+        /*
>+         * Objects with different numbers of fixed slots can be swapped only if they
>+         * are both shapeless non-natives, to preserve the invariant that objects with the
>+         * same shape have the same number of fixed slots.  Use a dynamic array for both.
>+         */
>+        JS_ASSERT(!isNative());
>+        JS_ASSERT(!other->isNative());
>+        size = sizeof(JSObject);
>+        if (!hasSlotsArray()) {
>+            if (!allocSlots(cx, numSlots()))
>+                return false;

Call generateOwnShape(cx) here.

>+        }
>+        if (!other->hasSlotsArray()) {
>+            if (!other->allocSlots(cx, other->numSlots()))
>+                return false;

Call other->generateOwnShape(cx) here.

>@@ -3498,23 +3534,22 @@ DefineStandardSlot(JSContext *cx, JSObje
>         JS_LOCK_OBJ(cx, obj);
>         if (!obj->ensureClassReservedSlots(cx)) {
>             JS_UNLOCK_OBJ(cx, obj);
>             return false;
>         }
> 
>         const Shape *shape = obj->nativeLookup(id);
>         if (!shape) {
>-            uint32 index = 2 * JSProto_LIMIT + key;
>-            if (!js_SetReservedSlot(cx, obj, index, v)) {
>+            uint32 slot = 2 * JSProto_LIMIT + key;
>+            if (!js_SetReservedSlot(cx, obj, slot, v)) {

We need a bug on this -- nothing AFAIK reads this "bank" of JSProto_LIMIT reserved slots. It is completely redundant with respect to the second bank (starting at JSProto_LIMIT, with the proto for key at JSProto_LIMIT + key). Can you confirm?

r=me with these addressed somehow. Did Igor or Gregor get a look at the GC changes?

/be

Comment 70

[Brendan Eich [:brendan]]

bhackett pointed out the non-native assertions for this and other in JSObject::swap, so no need to generateOwnShape. But I wonder what invariants (if any) non-natives might expect to be maintained about slots/fixedSlots relations across a swap...

/be

Comment 71

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/tracemonkey/rev/c45685276ce5

Comment 72

[Nicholas Nethercote [inactive]]

Leaky, leaky (this from running trace-tests on a machine with Valgrind installed):

==7732== 24 bytes in 1 blocks are definitely lost in loss record 1 of 6
==7732==    at 0x47E925F: calloc (vg_replace_malloc.c:467)
==7732==    by 0x805ABCF: js_calloc (jsutil.h:205)
==7732==    by 0x807779A: JSRuntime::calloc(unsigned int, JSContext*) (jscntxt.h:1712)
==7732==    by 0x8077B6D: JSContext::calloc(unsigned int) (jscntxt.h:2309)
==7732==    by 0x80797BD: JSObject::getEmptyShape(JSContext*, js::Class*, unsigned int) (jsscopeinlines.h:71)
==7732==    by 0x80D6955: js::InitScopeForObject(JSContext*, JSObject*, js::Class*, JSObject*, js::gc::FinalizeKind) (jsobjinlines.h:810)
==7732==    by 0x80DFB6C: JSObject* js::detail::NewObject<false, true>(JSContext*, js::Class*, JSObject*, JSObject*, js::gc::FinalizeKind) (jsobjinlines.h:1019)
==7732==    by 0x80D6C5D: js::NewFunction(JSContext*, JSObject*) (jsobjinlines.h:1037)
==7732==    by 0x80DDEFF: js_NewFunction(JSContext*, JSObject*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, unsigned int, JSObject*, JSAtom*) (jsfun.cpp:2832)
==7732==    by 0x80DE6BD: js_DefineFunction(JSContext*, JSObject*, JSAtom*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, unsigned int) (jsfun.cpp:3019)
==7732==    by 0x8071C3E: JS_DefineFunction (jsapi.cpp:4404)
==7732==    by 0x8071B7C: JS_DefineFunctions (jsapi.cpp:4389)
==7732== 
==7732== 24 bytes in 1 blocks are definitely lost in loss record 2 of 6
==7732==    at 0x47E925F: calloc (vg_replace_malloc.c:467)
==7732==    by 0x805ABCF: js_calloc (jsutil.h:205)
==7732==    by 0x807779A: JSRuntime::calloc(unsigned int, JSContext*) (jscntxt.h:1712)
==7732==    by 0x8077B6D: JSContext::calloc(unsigned int) (jscntxt.h:2309)
==7732==    by 0x80797BD: JSObject::getEmptyShape(JSContext*, js::Class*, unsigned int) (jsscopeinlines.h:71)
==7732==    by 0x811FE89: js_InitClass(JSContext*, JSObject*, JSObject*, js::Class*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, JSPropertySpec*, JSFunctionSpec*, JSPropertySpec*, JSFunctionSpec*) (jsobj.cpp:3713)
==7732==    by 0x8180549: js_InitRegExpClass(JSContext*, JSObject*) (jsregexp.cpp:901)
==7732==    by 0x8120776: js_GetClassObject(JSContext*, JSObject*, JSProtoKey, JSObject**) (jsobj.cpp:3938)
==7732==    by 0x8120A2A: js_FindClassObject(JSContext*, JSObject*, JSProtoKey, js::Value*, js::Class*) (jsobj.cpp:4003)
==7732==    by 0x8125745: js::FindClassPrototype(JSContext*, JSObject*, JSProtoKey, JSObject**, js::Class*) (jsobj.cpp:5885)
==7732==    by 0x805CB49: js::NewBuiltinClassInstance(JSContext*, js::Class*, js::gc::FinalizeKind) (jsobjinlines.h:922)
==7732==    by 0x805CBBB: js::NewBuiltinClassInstance(JSContext*, js::Class*) (jsobjinlines.h:933)
==7732== 
==7732== 24 bytes in 1 blocks are definitely lost in loss record 3 of 6
==7732==    at 0x47E925F: calloc (vg_replace_malloc.c:467)
==7732==    by 0x805ABCF: js_calloc (jsutil.h:205)
==7732==    by 0x807779A: JSRuntime::calloc(unsigned int, JSContext*) (jscntxt.h:1712)
==7732==    by 0x8077B6D: JSContext::calloc(unsigned int) (jscntxt.h:2309)
==7732==    by 0x80797BD: JSObject::getEmptyShape(JSContext*, js::Class*, unsigned int) (jsscopeinlines.h:71)
==7732==    by 0x811FE89: js_InitClass(JSContext*, JSObject*, JSObject*, js::Class*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, JSPropertySpec*, JSFunctionSpec*, JSPropertySpec*, JSFunctionSpec*) (jsobj.cpp:3713)
==7732==    by 0x811F723: js_InitObjectClass(JSContext*, JSObject*) (jsobj.cpp:3506)
==7732==    by 0x806B19B: js_InitFunctionAndObjectClasses(JSContext*, JSObject*) (jsapi.cpp:1408)
==7732==    by 0x8120776: js_GetClassObject(JSContext*, JSObject*, JSProtoKey, JSObject**) (jsobj.cpp:3938)
==7732==    by 0x8120A2A: js_FindClassObject(JSContext*, JSObject*, JSProtoKey, js::Value*, js::Class*) (jsobj.cpp:4003)
==7732==    by 0x8125745: js::FindClassPrototype(JSContext*, JSObject*, JSProtoKey, JSObject**, js::Class*) (jsobj.cpp:5885)
==7732==    by 0x812594F: js_GetClassPrototype(JSContext*, JSObject*, JSProtoKey, JSObject**, js::Class*) (jsobj.cpp:5932)
==7732== 
==7732== 24 bytes in 1 blocks are definitely lost in loss record 4 of 6
==7732==    at 0x47E925F: calloc (vg_replace_malloc.c:467)
==7732==    by 0x805ABCF: js_calloc (jsutil.h:205)
==7732==    by 0x807779A: JSRuntime::calloc(unsigned int, JSContext*) (jscntxt.h:1712)
==7732==    by 0x8077B6D: JSContext::calloc(unsigned int) (jscntxt.h:2309)
==7732==    by 0x80797BD: JSObject::getEmptyShape(JSContext*, js::Class*, unsigned int) (jsscopeinlines.h:71)
==7732==    by 0x811FE89: js_InitClass(JSContext*, JSObject*, JSObject*, js::Class*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, JSPropertySpec*, JSFunctionSpec*, JSPropertySpec*, JSFunctionSpec*) (jsobj.cpp:3713)
==7732==    by 0x806D8D3: JS_InitClass (jsapi.cpp:2876)
==7732==    by 0x82C2EBF: JS::RegisterPerfMeasurement(JSContext*, JSObject*) (jsperf.cpp:258)
==7732==    by 0x805621C: NewGlobalObject(JSContext*) (js.cpp:5225)
==7732==    by 0x805641F: Shell(JSContext*, int, char**, char**) (js.cpp:5253)
==7732==    by 0x80566D5: main (js.cpp:5415)
==7732== 
==7732== 24 bytes in 1 blocks are definitely lost in loss record 5 of 6
==7732==    at 0x47E925F: calloc (vg_replace_malloc.c:467)
==7732==    by 0x805ABCF: js_calloc (jsutil.h:205)
==7732==    by 0x807779A: JSRuntime::calloc(unsigned int, JSContext*) (jscntxt.h:1712)
==7732==    by 0x8077B6D: JSContext::calloc(unsigned int) (jscntxt.h:2309)
==7732==    by 0x80797BD: JSObject::getEmptyShape(JSContext*, js::Class*, unsigned int) (jsscopeinlines.h:71)
==7732==    by 0x811FE89: js_InitClass(JSContext*, JSObject*, JSObject*, js::Class*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, JSPropertySpec*, JSFunctionSpec*, JSPropertySpec*, JSFunctionSpec*) (jsobj.cpp:3713)
==7732==    by 0x808CA4E: js_InitArrayClass(JSContext*, JSObject*) (jsarray.cpp:2961)
==7732==    by 0x8120776: js_GetClassObject(JSContext*, JSObject*, JSProtoKey, JSObject**) (jsobj.cpp:3938)
==7732==    by 0x8120A2A: js_FindClassObject(JSContext*, JSObject*, JSProtoKey, js::Value*, js::Class*) (jsobj.cpp:4003)
==7732==    by 0x8125745: js::FindClassPrototype(JSContext*, JSObject*, JSProtoKey, JSObject**, js::Class*) (jsobj.cpp:5885)
==7732==    by 0x812594F: js_GetClassPrototype(JSContext*, JSObject*, JSProtoKey, JSObject**, js::Class*) (jsobj.cpp:5932)
==7732==    by 0x808D6D5: JSObject* js::detail::NewObject<false, false>(JSContext*, js::Class*, JSObject*, JSObject*, js::gc::FinalizeKind) (jsobjinlines.h:990)
==7732== 
==7732== 24 bytes in 1 blocks are definitely lost in loss record 6 of 6
==7732==    at 0x47E925F: calloc (vg_replace_malloc.c:467)
==7732==    by 0x805ABCF: js_calloc (jsutil.h:205)
==7732==    by 0x807779A: JSRuntime::calloc(unsigned int, JSContext*) (jscntxt.h:1712)
==7732==    by 0x8077B6D: JSContext::calloc(unsigned int) (jscntxt.h:2309)
==7732==    by 0x80797BD: JSObject::getEmptyShape(JSContext*, js::Class*, unsigned int) (jsscopeinlines.h:71)
==7732==    by 0x811FE89: js_InitClass(JSContext*, JSObject*, JSObject*, js::Class*, int (*)(JSContext*, unsigned int, js::Value*), unsigned int, JSPropertySpec*, JSFunctionSpec*, JSPropertySpec*, JSFunctionSpec*) (jsobj.cpp:3713)
==7732==    by 0x806D8D3: JS_InitClass (jsapi.cpp:2876)
==7732==    by 0x81DE685: js::InitJITStatsClass(JSContext*, JSObject*) (jstracer.cpp:445)
==7732==    by 0x804CEE2: ProcessArgs(JSContext*, JSObject*, char**, int) (js.cpp:741)
==7732==    by 0x80564FA: Shell(JSContext*, int, char**, char**) (js.cpp:5307)
==7732==    by 0x80566D5: main (js.cpp:5415)

Comment 73

[Brian Hackett [Laid off!]]

Will fix the leak, here's a followup to a different bug, use of uninitialized variable:

http://hg.mozilla.org/tracemonkey/rev/e613a2251de9

Comment 74

[Brian Hackett [Laid off!]]

Here is the memory leak fix:

http://hg.mozilla.org/tracemonkey/rev/df2e888625df

Comment 75

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c45685276ce5

Comment 76

[chris hofmann]

have a look at possible crash regressions in bug 606536