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.

(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.

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.

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

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!

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 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.

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.

(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.

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

(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.

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?

(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

(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.

> 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 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.)

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 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.

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 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.

billm: you attached the wrong patch.

(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.

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

(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.

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 on attachment 533312 [details] [diff] [review] cell iterations patch Nice!

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 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.

(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 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 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.

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

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?

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

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

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

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

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.

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.

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