571249 - Add memory reporters for JSScripts, non-fixed object slot arrays, and string chars

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We now have (in tracemonkey branch) a generic JS gc chunks memory reporter, but there are two more things that would be good to have:

- a reporter for the runtime's malloc bytes counter (easy)

- a reporter that tallies up the size of the contents of gc objects, including strings and array contents (would be nice if this was split up a little more too, e.g. separate strings out)

bug 560818 added the gc chunks counter, which involved igor's piece in JS and a corresponding piece in xpconnect to actually create the reporter; the above should probably follow this.

Comment 1

[Igor Bukanov]

(In reply to comment #0)
> - a reporter that tallies up the size of the contents of gc objects, including
> strings and array contents (would be nice if this was split up a little more
> too, e.g. separate strings out)

It is pretty straightforward to enumerate the heap to get such counting information. A proper locking could be involved, but there should be no problems with doing that. But such enumeration could be rather lengthy in time roughly on the scale of the GC run. So perhaps it would be better to provide only the accounting assembled during the last GC?

> 
> bug 560818 added the gc chunks counter, which involved igor's piece in JS and a
> corresponding piece in xpconnect to actually create the reporter; the above
> should probably follow this.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Expensive is fine -- these things are queried on-demand, so they're never going to be in any critical performance path.  Shaver has some ideas about starting the traverse at different roots as well to try to figure out which page/tab/etc. is holding on to the most memory, I'll let him chime in here.

Comment 3

[Mike Shaver (:shaver emeritus)]

Yeah, I'll sketch this up -- I guess this bug is about the type-classified walk of the GC heap?

Comment 5

[Nicholas Nethercote [inactive]]

I'll take this one.  In particular, I'm interested in counting the following things that aren't on the GC heap, and aren't currently covered by about:memory:

- String chars

- Non-fixed object slot arrays

- JSScripts

Measurements with Massif in bug 563700 have shown that these can be a decent chunk of the (malloc) heap.  I've renamed this bug to be more specific about that.

Doing a GC heap traversal seems the best way to do this.  And by "traversal" I mean iterating over all the arenas, looking at used slots, as opposed to a GC-style trace which would only get the live objects.  billm volunteered to write some code to do such a traversal to get things going.  Thanks, billm!

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: patch, v1

This patch implements the js/scripts reporter properly.  The numbers I'm getting for scripts match pretty closely to what Massif was telling me, which is encouraging.

The patch also adds stub reporters for js/string-chars and js/objects-slots which just return 0;  these will need the code from billm.

Igor, can you take a look at GetCompartmentScriptsSize?  I iterate over all the JSScripts in the compartment, just reading the .size field of each one.  Do I need a JS::AutoEnterScriptCompartment in there?  The JSScript-traversal code I copied from jsdbgapi.cpp had one, but it was doing much more complicated stuff.

Comment 7

[Igor Bukanov]

Comment on attachment 532107 [details] [diff] [review]
patch, v1

>diff --git a/js/src/jsscript.h b/js/src/jsscript.h
>--- a/js/src/jsscript.h
>+++ b/js/src/jsscript.h
>@@ -413,22 +413,23 @@ struct JSScript {
>                                uint16 nClosedArgs, uint16 nClosedVars, JSVersion version);
> 
>     static JSScript *NewScriptFromCG(JSContext *cx, JSCodeGenerator *cg);
> 
>     /* FIXME: bug 586181 */
>     JSCList         links;      /* Links for compartment script list */
>     jsbytecode      *code;      /* bytecodes and their immediate operands */
>     uint32          length;     /* length of code vector */
>+    uint32          size;       /* size of the entire JSScript */

I do not see why it is not possible to recover script size from other fields. Besides, the malloc size does not include the size of other malloc things the script allocate. So what we should do is to fix JS_GetScriptTotalSize to return the correct value.

>+static PRInt64
>+GetCompartmentScriptsSize(JSCompartment *c)
>+{

Why the size is PRInt64 and not size_t? We cannot allocate over 4GB of scripts on 32 bit CPU!

>+    PRInt64 n = 0;
>+    for (JSScript *script = (JSScript *)c->scripts.next;
>+         &script->links != &c->scripts;
>+         script = (JSScript *)script->links.next)
>+    {
>+        n += script->size; 
>+    }

I am not sure what do you want here: is the desired value is the current size of all allocated scripts or only live scripts? A similar question is about strings.

Comment 8

[Igor Bukanov]

Ok, I see the need is to cover everything. If so, then to calculate sizes we need to enumerate all things in all GC arenas ignoring the things on the free list. During such enumeration we can account for scripts and strings as necessary without the need to account for scripts using a separated loop.

The complication is the background finalization. For simplicity I suppose we can just wait until that finishes and then freeze the world during heap enumeration in a manner similar to what JS_GC or JS_TraceRuntime is doing.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> >+    uint32          size;       /* size of the entire JSScript */
> 
> I do not see why it is not possible to recover script size from other
> fields. Besides, the malloc size does not include the size of other malloc
> things the script allocate. So what we should do is to fix
> JS_GetScriptTotalSize to return the correct value.

It struck me as possible but an enormous pain.  What other things are malloc'd for the script?  I don't see any other allocations in NewScript(), are there some elsewhere?

> >+static PRInt64
> >+GetCompartmentScriptsSize(JSCompartment *c)
> >+{
> 
> Why the size is PRInt64 and not size_t? We cannot allocate over 4GB of
> scripts on 32 bit CPU!

Because the nsIMemoryReporter interface uses PRInt64.

> I am not sure what do you want here: is the desired value is the current
> size of all allocated scripts or only live scripts? A similar question is
> about strings.

In both cases I want all allocated ones.

Comment 10

[Nicholas Nethercote [inactive]]

Igor, thanks for the feedback, but you didn't answer the question I asked in comment 7.  Can you?

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> Igor, thanks for the feedback, but you didn't answer the question I asked in
> comment 7.  Can you?

JS::AutoEnterScriptCompartment is not necessary there. But I see no point in doing a separated script traversal. It can be done during the arena traversal that is necessary to account for object slots etc.

Comment 12

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

Attachment: patch to iterate over allocated objects

Here's a proof of concept for iterating over every object. I haven't really tested it. Also, I'm not sure I've gotten all the locking stuff right. Could you give it a quick look, Igor?

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Here's a proof of concept for iterating over every object. I haven't really
> tested it. Also, I'm not sure I've gotten all the locking stuff right. Could
> you give it a quick look, Igor?

1. With bug 601234 landed the code needs to deal with free lists decoupled from the the original arena.

2. The patch should assert that the accounting is not called from the GC.

Otherwise this looks reasonable. Also the bug 656261

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> Also the bug 656261

I mean the bug 656261 conflicts with this, but it is straightforward to address that replacing the template switch with a simple loop.

Comment 15

[Nicholas Nethercote [inactive]]

> I see no point in
> doing a separated script traversal. It can be done during the arena
> traversal that is necessary to account for object slots etc.

Oh?  My understanding is that scripts for a compartment are stored in JSCompartment::scripts, which is entirely separate from the compartment's arenas.  I must be missing something, please enlighten me :)

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: patch v2

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 532877 [details] [diff] [review]
patch v2

I'm requesting review from Luke specifically for MeasureStringChars and the related jsstr.h changes, and from Igor for everything else.

LUke, please note the "njn" comment at the top of MeasureStringChars;  suggestions for a better way to structure this function would be welcome.  Also note that we reach each string by traversing all things in the GC arenas, not by doing a GC-style liveness trace.  (This affects how things like dependent strings are handled.)

Comment 18

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

Attachment: fix for igor's comments

Sorry Nick, I didn't get time to address Igor's comments. Here's an interdiff. I think the purge call should be enough to fix the freelist issue.

Comment 19

[Igor Bukanov]

Comment on attachment 533009 [details] [diff] [review]
fix for igor's comments

>     for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); ++c) {
>         JSCompartment *comp = *c;
>+        comp->freeLists.purge();

This makes the lists unusable for farther allocations. A proper fix can, for example, copy the free lists to arenas and then clear gthe corresponding arenas again after the loop.  This way the GC memory observation can avoid influencing the memory allocations.

Comment 20

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

Attachment: correct fix

Yes, you're right. I forgot that the GC completely rebuilds the arena list after purge.

I think this should fix the problem. Rather than changing the freeLists, it just accounts for them in the loop.

Comment 21

[Igor Bukanov]

Comment on attachment 532877 [details] [diff] [review]
patch v2

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp

>+void
>+IterateObjects(JSContext *cx, void *data, IterateCallback callback)
>+{
...
>+    AutoLockGC lock(rt);

Add an assert here that !rt->gcRunning with a comment that this should not be called during the GC.

>+    AutoGCSession gcsession(cx);
>+    AutoUnlockGC unlock(rt);
>+
>+#ifdef JS_THREADSAFE
>+    rt->gcHelperThread.waitBackgroundSweepEnd(rt);
>+#endif

Nit: Pass false as the second argument to waitBackgroundSweepEnd and swap it with unlock.

>diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp
>@@ -1085,16 +1085,17 @@ JSScript::NewScript(JSContext *cx, uint3
...
>     PodZero(script);
>     script->length = length;
>+    script->size = size;

As I wrote, I do not understand the need for this size field. What prevents recalculating it from other information stored in the script?

>diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp
>+void
>+GetJSObjectSlotsCallback(JSContext *cx, void *v, size_t thingKind, void *thing)
>+{
>+    switch (thingKind) {
>+      case JSTRACE_OBJECT: {
>+        JSObject *obj = (JSObject *)thing;
>+        if (obj->hasSlotsArray()) {

This does not account for typed arrays, regular expressions and other objects where we use malloced data. Also I do not see why implementing a separated callback to measure strings is necessary. I.e. why not return a struct with counters to report filled during single IterateOverObjects invocation? Also such reporting function can live in jsdgapi so xpconnect can just call it to get the necessary information.

Comment 22

[Nicholas Nethercote [inactive]]

billm: you attached the wrong patch.

Comment 23

[Nicholas Nethercote [inactive]]

(In reply to comment #21)

> As I wrote, I do not understand the need for this size field. What prevents
> recalculating it from other information stored in the script?

You can't tell how long the srcnotes section is.  In other words, this expression would suffice:

  script->code + script->length * sizeof(jsbytecode) +
  script->nsrcnotes * sizeof(jssrcnote) - (uint8 *)script;

But script->nsrcnotes does not exist.  We could add it, but instead of doing that we might as well script->size which gives me what I want directly.


> >diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp
> >+void
> >+GetJSObjectSlotsCallback(JSContext *cx, void *v, size_t thingKind, void *thing)
> >+{
> >+    switch (thingKind) {
> >+      case JSTRACE_OBJECT: {
> >+        JSObject *obj = (JSObject *)thing;
> >+        if (obj->hasSlotsArray()) {
> 
> This does not account for typed arrays, regular expressions and other
> objects where we use malloced data.

True.  It's not meant to.  Neither of those cases have come up as significant causes of memory usage so far but I guess they'll be good to keep in mind for the future.

What other object kinds do we malloc data for, BTW?


> Also I do not see why implementing a
> separated callback to measure strings is necessary. I.e. why not return a
> struct with counters to report filled during single IterateOverObjects
> invocation?

It doesn't fit with nsIMemoryReporter.  Each memory reporter reports a single figure, and it has a function GetMemoryUsed() that computes that figure.  There's no way to compute multiple figures at once and then report them back to multiple reporters.  So if I had a single callback, it would compute multiple numbers on each heap traversal but each time all but one of those numbers would be discarded.  about:memory is the only consumer and it isn't performance critical but avoiding this egregious re-computation seems desirable.

(Perhaps a better solution is to change IterateObjects to take an input parameter that lets you restrict the heap traversal to particular traceKinds, eg. to just strings, or just objects.  That would avoid the unnecessary re-traversals.)


> Also such reporting function can live in jsdgapi so xpconnect
> can just call it to get the necessary information.

Which functions do you think I should move into jsdbgapi?

Another question: the JS_NewContext() calls in GetJSStringChars and GetJSObjectSlots are ugly.  Can something nicer be done there?


billm:
- I'm planning to rename IterateObjects as IterateThings because it seems more accurate.

- I also hoisted the initialization of thingKind in IterateArenaThings.

- Eventually I'm going to want to do this kind of heap traversal on a per-compartment basis.  I guess it'll be possible to split out the body of the loop in IterateThings so that it can be called for a single compartment.

Comment 24

[Nicholas Nethercote [inactive]]

BTW, luke, I'd still like a review for the string parts even though Igor gave an r-, if that's ok!

Comment 25

[Igor Bukanov]

(In reply to comment #23)
> But script->nsrcnotes does not exist.  We could add it, but instead of doing
> that we might as well script->size which gives me what I want directly.

js_XDRScript in http://hg.mozilla.org/tracemonkey/file/49b491d2bcc8/js/src/jsscript.cpp#l474 calculates the size of the notes on the fly. So lets move that to a separated function and use that in size caculations.

> > >diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp
> > >+void
> > >+GetJSObjectSlotsCallback(JSContext *cx, void *v, size_t thingKind, void *thing)
> > >+{
> > >+    switch (thingKind) {
> > >+      case JSTRACE_OBJECT: {
> > >+        JSObject *obj = (JSObject *)thing;
> > >+        if (obj->hasSlotsArray()) {
> > 
> > This does not account for typed arrays, regular expressions and other
> > objects where we use malloced data.
> 
> True.  It's not meant to.  Neither of those cases have come up as
> significant causes of memory usage so far but I guess they'll be good to
> keep in mind for the future.
> 
> What other object kinds do we malloc data for, BTW?

typed arrays, exceptions (the stored stack trace can be big), xml, local name data structures in functions, weak maps. 

> It doesn't fit with nsIMemoryReporter.  Each memory reporter reports a
> single figure, and it has a function GetMemoryUsed() that computes that
> figure.  There's no way to compute multiple figures at once and then report
> them back to multiple reporters.
...
> Which functions do you think I should move into jsdbgapi?

I would like to see something like:

size_t JS_GetMallocAllocationSize(JSContext *cx, JSAllocationKind mallocKind) where JSAllocationKind specifies the desired constants like string data, object slots etc., i.e. whatever we find useful plus a constant to represent the sum of everything.

Then the GC thing iterator callback can check for the enum and skip some parts of calculations.

> Another question: the JS_NewContext() calls in GetJSStringChars and
> GetJSObjectSlots are ugly.  Can something nicer be done there?

The reason the patch takes JSContext* and not JSRuntime* is that AutoGCSession has to access  the JSThreadData for the current thread. That can be found using the current thread id so in principle we can remove that. But then a few things regarding the trace monitor access in AutoGCSession::AutoGCSession should be patched to access JSThreadData, not cx.

Comment 26

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

Attachment: cell iterations patch

Wow, I can't do anything right in this bug. Here's the right patch. I also added the functionality for restricting the iteration to a particular compartment or mix of thingKinds. I renamed the function to IterateCells, which I like better than IterateThings.

I'll have to update it again once bug 656261, but that will just clean things up.

Comment 27

[Igor Bukanov]

Comment on attachment 533312 [details] [diff] [review]
cell iterations patch

Nice!

Comment 28

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

Attachment: cell iteration patch v3

This version just switches the interface to take a trace kind mask instead of a finalize kind mask. I think it makes sense to avoid exposing finalize kinds too far outside the GC.

Comment 29

[Nicholas Nethercote [inactive]]

Comment on attachment 533475 [details] [diff] [review]
cell iteration patch v3

I spun off bug 658137 for the GC iteration patch.  This bug can now just be about the memory reporters.

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: patch v3

(In reply to comment #25)
> 
> js_XDRScript in
> http://hg.mozilla.org/tracemonkey/file/49b491d2bcc8/js/src/jsscript.cpp#l474
> calculates the size of the notes on the fly. So lets move that to a
> separated function and use that in size caculations.

Done.
 
> I would like to see something like:
> 
> size_t JS_GetMallocAllocationSize(JSContext *cx, JSAllocationKind
> mallocKind) where JSAllocationKind specifies the desired constants like
> string data, object slots etc., i.e. whatever we find useful plus a constant
> to represent the sum of everything.
> 
> Then the GC thing iterator callback can check for the enum and skip some
> parts of calculations.

Bill's addition of the traceKindMask argument to IterateCells() achieves a similar effect -- the callbacks are nice and small now, and we only iterate over the arenas we need to.  It doesn't seem worthwhile to introduce an extra function and an extra type, so I'll leave this code in xpcjsruntime.cpp.


Luke, review is still needed for the string-related parts.

Comment 31

[Igor Bukanov]

Comment on attachment 533510 [details] [diff] [review]
patch v3

>+size_t
>+JSScript::totalSize()
>+{
>+    return code +
>+           length * sizeof(jsbytecode) +
>+           numNotes() * sizeof(jssrcnote) -
>+           (uint8 *)this;
>+}

Nit: space between the cast () and its operand.

>diff --git a/js/src/jsscript.h b/js/src/jsscript.h
>@@ -414,21 +414,24 @@ struct JSScript {
>     jsbytecode      *code;      /* bytecodes and their immediate operands */
>     uint32          length;     /* length of code vector */
> 
>+    size_t          totalSize();/* Size of the JSScript and all sections */
>+    uint32          numNotes(); /* Number of entries in the srcnotes section */
>+
>   private:
>+    size_t          callCount_; /* Number of times the script has been called. */
>+
>     uint16          version;    /* JS version under which script was compiled */
> 
>-    size_t          callCount_; /* Number of times the script has been called. */
>-
>   public:
>     uint16          nfixed;     /* number of slots besides stack operands in
>                                    slot array */

Nit: move the methods after all the fields so it is easier to see the layout. Also move callCont_ after the nfixed for a better layout on 64 bit.  r+ with that fixed on non-string related changes.

Comment 32

[Luke Wagner [:luke]]

Comment on attachment 533510 [details] [diff] [review]
patch v3

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

Sorry for the delay

::: js/src/jsstr.h
@@ +354,5 @@
>      JS_ALWAYS_INLINE
> +    bool isInline() const {
> +        return (d.lengthAndFlags & FLAGS_MASK) == INLINE_FLAGS &&
> +               d.u1.chars == d.inlineStorage;
> +    }

This will catch JSInlineStrings but not JSShortStrings.  I checked and the encoding comment here is misleading.  The underlying reason is that there are two pieces of info we want for each type: how we represent instances of the type and how we test "is type X or a subtype of X".  I just made a patch which does this that I'll post next.

::: js/src/xpconnect/src/xpcjsruntime.cpp
@@ +1356,5 @@
> +    return data.n;
> +}
> +
> +// njn: This function needs to do something different for pretty much every kind
> +// of string.  Using chains of isX() functions is an awkward way to do it.

I opened this along side the ASCII-art string hierarchy and I thought this code followed as a straightforward case analysis.  I only see one alternative, which is to have a getType() which returns an enum you can switch over.  I'm not sure that would be any more straightforward, particularly in view of my next comment:

@@ +1360,5 @@
> +// of string.  Using chains of isX() functions is an awkward way to do it.
> +//
> +// This measures the heap memory used by the string's chars.
> +static PRInt64
> +MeasureStringChars(JSString *str)

Good function, but perhaps we can make this a JS_FRIEND_API function exported by js and implemented in jsstr.cpp (some day the vm/String module in view of bug 653057).  The reason is that the case analysis depends on implementation details of the string hierarchy and thus anyone who updates strings should update this as well.  With the code over in xpconnect one is more likely to forget.  Also, its nice to have this build with libjs.

Comment 33

[Luke Wagner [:luke]]

Attachment: JSString::isInline() and comment

Said patch.  If you hg revert jsstr.h in your patch and apply this, everything should work.

Comment 34

[Nicholas Nethercote [inactive]]

Comment on attachment 533669 [details] [diff] [review]
JSString::isInline() and comment

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

r=me.  There are some nits in the big comment, I'll fold in them into my next version of the patch.

::: js/src/jsstr.h
@@ +191,5 @@
> +     * encoding" entry for a type specifies the flag bits used to create a
> +     * string instance of that type. Abstract types have no instances and thus
> +     * have no such entry. The "subtype predicate" entry for a type specifies
> +     * the predicate used to query whether a JSString instance is subtype
> +     * (reflexively) of that type.

This comment is much better than the old one, thanks for that.

@@ +202,5 @@
> +     *   Linear       -          xxx0
> +     *   Dependent    0010       xx1x
> +     *   Flat         -          xx00
> +     *   Extensible   1100       1100
> +     *   Fixed        0100       xx00 and xx != 11

predicate should be:  isFlat && !isExtensible

@@ +203,5 @@
> +     *   Dependent    0010       xx1x
> +     *   Flat         -          xx00
> +     *   Extensible   1100       1100
> +     *   Fixed        0100       xx00 and xx != 11
> +     *   Inline       0100       xx00 u1.chars == inlineStorage or is Short

predicate should be: isFixed && (u1.chars == inlineStorage || isShort)

@@ +209,5 @@
> +     *   External     0100       xxxx header in FINALIZE_EXTERNAL_STRING arena
> +     *   Atom         1000       x000
> +     *   InlineAtom   1000       1000 and is Inline
> +     *   ShortAtom    1000       1000 and is Short
> +     *   Static       0000       0000

It's StaticAtom, not Static.

Hmm, isn't it true that all JSStaticAtoms are also inline?  So it should be renamed JSStaticInlineAtom?  I guess you could argue that although all static atoms are currently inline they don't have to be?

Comment 35

[Luke Wagner [:luke]]

Sounds good.  Until (if ever) there is a bifurcation into a short and non-short static atom, I'd prefer JSStaticAtom.

Comment 36

[Nicholas Nethercote [inactive]]

Attachment: string changes only, v4

I've split the patch into two.  This is the string changes.  Carrying the r+ over from earlier.

Comment 37

[Nicholas Nethercote [inactive]]

Attachment: memory reporters part, v4

Carrying r+ over from before.  numNotes() was busted in the old version and caused browser crashes;  this version is better.

Comment 38

[Nicholas Nethercote [inactive]]

NEXT ERROR PROCESS-CRASH | chrome://mochitests/content/chrome/toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul | application crashed (minidump found)
I'm getting crashes in js_DestroyContext on the try server, but only on Windows optimized builds (debug builds are fine).  Igor, any idea why this might happen?


Operating system: Windows NT
                  5.1.2600 Service Pack 2
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXCEPTION_ACCESS_VIOLATION_WRITE
Crash address: 0x641b81e0

Thread 0 (crashed)
 0  mozjs.dll!js_DestroyContext(JSContext *,JSDestroyContextMode) [jscntxt.cpp:6d606f52e375 : 588 + 0xc]
    eip = 0x00583ecc   esp = 0x0012cbc8   ebp = 0x0012cbdc   ebx = 0x0dbfcc40
    esi = 0x0dbfcc40   edi = 0x0212c000   eax = 0x641b81dc   ecx = 0x0dbfcbd1
    edx = 0x0165b0ec   efl = 0x00010202
    Found by: given as instruction pointer in context
 1  mozjs.dll!JS_DestroyContextNoGC [jsapi.cpp:6d606f52e375 : 1037 + 0xa]
    eip = 0x005866ab   esp = 0x0012cbe4   ebp = 0x0012cc18
    Found by: previous frame's frame pointer
 2  xul.dll!GetJSObjectSlots [xpcjsruntime.cpp:6d606f52e375 : 1354 + 0x6]
    eip = 0x10873d94   esp = 0x0012cbec   ebp = 0x0012cc18
    Found by: call frame info
 3  xul.dll!MemoryReporter_XPConnectJSObjectSlots::GetMemoryUsed(__int64 *) [xpcjsruntime.cpp:6d606f52e375 : 1405 + 0x4]
    eip = 0x10873f78   esp = 0x0012cc20   ebp = 0x0012cc30
    Found by: previous frame's frame pointer

Comment 39

[Nicholas Nethercote [inactive]]

I've spun off bug 662963 for the jsstr.{cpp,h} changes, so I can land them independently of working out the Windows crash in the XPConnect reporters.

Comment 40

[Nicholas Nethercote [inactive]]

I landed this with the object-slots and string-chars reporters turned off, because the lack of progress was blocking bug 661474:

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

I filed bug 664647 as a follow-up to enable those reporters.

Comment 41

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/b35005673847