Closed
Bug 394853
Opened 17 years ago
Closed 17 years ago
Leak detection helper for js shell
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
Attachments
(1 file, 8 obsolete files)
21.78 KB,
patch
|
brendan
:
review+
brendan
:
approval1.9+
|
Details | Diff | Splinter Review |
To simplify regression tests for bugs like 394709 it would be nice to have a function is jsshell that returns the current heap size,
Assignee | ||
Comment 1•17 years ago
|
||
The patch adds gcstats() function to the shell that return an object with stats about gc heap. For now the object contains the single property, heapsize with the usage demonstrated by the following example: js> gcstats().heapsize 27696 js> for (var i = 0; i != 100*1000; ++i) var o = {}; js> gcstats().heapsize 3748192 js> gc(); before 3748192, after 36928, break 0860a000 js> gcstats().heapsize 36928 js> for (var i = 0; i != 100*1000; ++i) var o = {}; js> gcstats().heapsize 3748192 js> gc(); before 3748192, after 36928, break 0860a000 js> o = null; null js> gc(); before 36928, after 27696, break 0828a000 js> gcstats().heapsize 27696
Assignee | ||
Comment 2•17 years ago
|
||
Comment on attachment 279574 [details] [diff] [review] v1 It is not simple to build a leak detector using the patch. So I will morph the bug into a leak detector.
Attachment #279574 -
Flags: review?(brendan)
Assignee | ||
Updated•17 years ago
|
Attachment #279574 -
Attachment is obsolete: true
Assignee | ||
Comment 3•17 years ago
|
||
That gcstats that I proposed above required rather complex setup to use as a leak detector. A better idea would be to provide a tool that allows to observe when a particular object is GC-ed. The test case can then detect a leak when the object is not GC-ed.
Summary: function for js shell to return the heap size → Leak detection helper for for js shel
Assignee | ||
Updated•17 years ago
|
Summary: Leak detection helper for for js shel → Leak detection helper for js shell
Assignee | ||
Comment 4•17 years ago
|
||
The patch adds DetectGC class to the shell. When the constructor is called as a function, it returns the number of finalized so far DetectGC objects. Here is a shell session to demonstrate the usage: ~> js js> var obj = { a: new DetectGC() }; js> gc(); before 27696, after 27696, break 08cb0000 js> DetectGC() 0 js> obj.a = null; null js> gc(); before 27696, after 27696, break 08cb0000 js> DetectGC() 1 js>
Attachment #279611 -
Flags: review?(brendan)
Comment 5•17 years ago
|
||
Is it possible to detect, during shutdown, whether any objects leaked? For fuzzing, that would be better than adding DetectGC calls for specific objects and then trying to remove all references from the global object manually before shutdown.
Assignee | ||
Comment 6•17 years ago
|
||
(In reply to comment #5) > Is it possible to detect, during shutdown, whether any objects leaked? It is possible but it would not be that useful. For example, that watch point leak from bug 394709 is not visible during the shutdown as the code explicitly calls JS_ClearAllWatchPoints destroying all leak evidences. I.e. shutdown leaks, when there are no application roots, is just a subset of leaks in presence of long-living application roots like global scope for js shell or a window object for the browser. What can help is an option to count the number of live GC things to use it like in: run_potentially_leaking_code(); gc(); n = get_gc_thing_counter(); run_potentially_leaking_code(); gc(); if (n != get_gc_thing_counter()) throw "We got a leak"; Would it work for the fuzzier?
Comment 7•17 years ago
|
||
Why not add another function, "expectedGCDetections", which, when called with a number, causes jsshell to exit with an error code if, after JS_DestroyRuntime, DetectGC's jsrefcount isn't equal to that value?
Assignee | ||
Comment 8•17 years ago
|
||
(In reply to comment #7) > Why not add another function, "expectedGCDetections", which, when called with a > number, causes jsshell to exit with an error code if, after JS_DestroyRuntime, > DetectGC's jsrefcount isn't equal to that value? As I wrote in comment 6 that would not help the leak detector for bug 394709 as after JS_DestroyRuntime the counter would be the right one. At the moment the watch point list that stores the points will be destroyed.
Comment 9•17 years ago
|
||
What about if you added a DEBUG gcthing+JSString (I think that covers everything?) counter that's incremented for new gcthings+JSStrings and decremented for their collection, exposed via a DEBUG-only JS API? That would catch leaks generically without having to know the specific thing that leaks. (And yeah, you're right, if you know the thing that's leaked shutdown leak detection isn't interesting; it's only if you *don't* that it matters. Momentary thinko...)
Assignee | ||
Comment 10•17 years ago
|
||
(In reply to comment #9) > What about if you added a DEBUG gcthing+JSString (I think that covers > everything?) counter that's incremented for new gcthings+JSStrings and > decremented for their collection, exposed via a DEBUG-only JS API? Actually with the tracing API available one can count the number of live things even without DEBUG. So I will add a function that count the number possibly with an option to count only string/object/double etc.
Comment 11•17 years ago
|
||
I think having an object counter exposed would work for the fuzzer :)
Assignee | ||
Comment 12•17 years ago
|
||
Comment on attachment 279611 [details] [diff] [review] v2 New patch is comming
Attachment #279611 -
Flags: review?(brendan)
Assignee | ||
Comment 13•17 years ago
|
||
This patch adds gcthings function to count live GC things. Here is an example of its usage: ~> js js> gcthings() 374 js> gcthings("object") 104 js> gcthings("string") 172 js> gcthings("double") 1 js> gcthings("function") 98 js> gcthings("xml") 0 js> new Object(); [object Object] js> gcthings("object") 105 js> gc(); before 27696, after 27696, break 09428000 js> gcthings("object") 104 js> var o = null; for (var i = 0; i != 100*1000; ++i) o = {prev: o}; [object Object] js> gcthings("object") 100104 js> o = null; null js> gcthings("object") 100104 js> gc(); before 3748192, after 27696, break 09b95000 js> gcthings("object") 104 js> Observe here the effects of new born roots: to be reliable, call gc() before the function.
Attachment #279611 -
Attachment is obsolete: true
Attachment #279654 -
Flags: review?(brendan)
Assignee | ||
Comment 14•17 years ago
|
||
Among the 3 patches clearly wins on simplicity, see test cases in 394709. Now to make it work with the fuzzer using the pattern like: run_potentially_leaking_code(); gc(); var n = gcthings(); run_potentially_leaking_code(); if (n != gcthings()) throw "We got a leak"; it would be necessary to ensure that run_potentially_leaking_code(); does not adds new properties to the global object/window. I guess simple way to get this is to use evalcx function from jsshell.
Comment 15•17 years ago
|
||
Having a fix for bug 353737 would make it easier to fuzz with evalcx.
Comment 16•17 years ago
|
||
Bug 394967, too.
Comment 17•17 years ago
|
||
You attached the wrong patch as "v3" here. That's the patch for bug 394709.
Assignee | ||
Comment 18•17 years ago
|
||
Here is the right patch.
Attachment #279654 -
Attachment is obsolete: true
Attachment #279703 -
Flags: review?(brendan)
Attachment #279654 -
Flags: review?(brendan)
Comment 19•17 years ago
|
||
Neat! Here's one thing I noticed using it: the first syntax error, e.g. eval("/") leads to an increase in the number of gcthings. I wonder why.
Assignee | ||
Comment 20•17 years ago
|
||
(In reply to comment #19) > Here's one thing I noticed using it: the first syntax error, e.g. > eval("/") > leads to an increase in the number of gcthings. This is expected due to the lazy initialization of Error object constructors and prototypes.
Assignee | ||
Comment 21•17 years ago
|
||
Here is another demo of the effects of lazy initialization: ~> js js> gcthings("object") 104 js> gcthings("double") 1 js> gcthings("function") 98 js> Math [object Math] js> gcthings("object") 124 js> gcthings("double") 8 js> gcthings("function") 117 Here Math on the first access creates the object itself, 19 its function together with the corresponding wrapping objects, and 7 double constants.
Assignee | ||
Comment 22•17 years ago
|
||
The new patch extends gcthings with an option to count only things reachable from a particular thing. The idea behind that is to detect unwanted roots that precisely responsible for leaks. For example, the bug 394709 happens precisely because the watch point implementation added a new root that that can only be reclaimed when no other objects points to it resulting in a leak cycle. Thus the code like the following example should detect the leak even if potentially_leaking_code adds new objects to the global scope: gc(); var n = gcthings('all') - gcthings('all', this); potentially_leaking_code()' gc(); var n2 = gcthings('all') - gcthings('all', this); if (n != n2) throw "we got a leak" Here is a demo of gcthings with the new parameter. It shows the effects of atomized things that are not yet used by the global scope and the newborn roots. ~> js js> gcthings() 374 js> gcthings('all', this) 290 js> gcthings('objects') gcthings: trace kind name 'objects' is unknown js> gcthings('object') 104 js> gcthings('object', this) 104 js> new Object(); [object Object] js> gcthings('object') 105 js> gcthings('object', this) 104
Attachment #279703 -
Attachment is obsolete: true
Attachment #279729 -
Flags: review?(brendan)
Attachment #279703 -
Flags: review?(brendan)
Assignee | ||
Comment 23•17 years ago
|
||
Changes compared with v4: 1. For consistency with dumpHeap I renamed gcthings into countHeap with usage: js> help("countHeap", "dumpHeap") JavaScript-C 1.7 pre-release 3 2007-04-01 Command Usage Description ======= ===== =========== countHeap countHeap([start], [kind]) Count the number of live GC things or things reachable from start when it is given and is not null. kind is either 'all' (default) to count all things or one of 'object', 'double', 'string', 'function', 'qname', 'namespace', 'xml' to count only things of that kind dumpHeap dumpHeap([fileName], [start], [toFind], [maxDepth], [toIgnore]) Interface to JS_DumpHeap with output sent to file 2. help() implementation now contains explicit formating code so the usage explanation strings is readable both in the js.c source and when printed by help().
Attachment #279937 -
Flags: review?(brendan)
Assignee | ||
Updated•17 years ago
|
Attachment #279729 -
Attachment is obsolete: true
Attachment #279729 -
Flags: review?(brendan)
Assignee | ||
Comment 24•17 years ago
|
||
The new version uses consistent optional argument formating in help messages.
Attachment #279937 -
Attachment is obsolete: true
Attachment #279938 -
Flags: review?(brendan)
Attachment #279937 -
Flags: review?(brendan)
Comment 25•17 years ago
|
||
Comment on attachment 279938 [details] [diff] [review] v6 >+ JSCountHeapTracer *gtrc; g for global? Just wondering... >+static JSBool >+CountHeap(JSContext *cx, uintN argc, jsval *vp) >+{ >+ void* startThing = NULL; >+ int32 startTraceKind = 0; >+ int32 traceKind = -1, i; Hiding initialisation in declaration against house style, mostly -- sort of works here, but that little i at the end of the last line looks lost. >+ if (argc > 0 && vp[2 + 0] != JSVAL_NULL) { Use JS_ARGV as was done recenty elsewhere? >+ JS_TRACER_INIT(>rc.base, cx, CountHeapNotify); Add a blank line before this line? >- vp = &argv[0]; >- if (*vp != JSVAL_NULL && *vp != JSVAL_VOID) { >+ if (argc > 0 && *(vp = argv) != JSVAL_NULL) { > JSString *str; > >+ vp = argv; Redundant vp = argv -- the nexted assignment in the && right operand did it if control flow reaches here. But perhaps that nested assignment should be lost, since it is a bit ugly. >+static const char *const shell_help_messages[] = { >+#define H(usage, description) usage "\0" description H and the new ShowHelp* functions seem over-engineered, but now that they're written it is hard to say "no". Still can we not pick a maximum cmd length? /be
Assignee | ||
Comment 26•17 years ago
|
||
Fixing the nits and removing that over engineered show help.
Attachment #279938 -
Attachment is obsolete: true
Attachment #280038 -
Flags: review?(brendan)
Attachment #279938 -
Flags: review?(brendan)
Assignee | ||
Comment 27•17 years ago
|
||
Fixing inefficiency in CountHeap implementation: now the code adds node to the recycle list before calling JS_TraceChildren so it can be used immediately for the first traced child.
Attachment #280038 -
Attachment is obsolete: true
Attachment #280040 -
Flags: review?(brendan)
Attachment #280038 -
Flags: review?(brendan)
Comment 28•17 years ago
|
||
Comment on attachment 280040 [details] [diff] [review] v8 Thanks, r+a=me. /be
Attachment #280040 -
Flags: review?(brendan)
Attachment #280040 -
Flags: review+
Attachment #280040 -
Flags: approval1.9+
Assignee | ||
Comment 29•17 years ago
|
||
I checked in the patch from comment 27 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1189189440&maxdate=1189189663&who=igor%mir2.org
Assignee | ||
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•