394853 - Leak detection helper for js shell

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

To simplify regression tests for bugs like 394709 it would be nice to have a function is jsshell that returns the current heap size,

Comment 1

[Igor Bukanov]

Attachment: v1

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

Comment 2

[Igor Bukanov]

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.

Comment 3

[Igor Bukanov]

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.

Comment 4

[Igor Bukanov]

Attachment: v2

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>

Comment 5

[Jesse Ruderman]

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.

Comment 6

[Igor Bukanov]

(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

[Jeff Walden [:Waldo]]

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?

Comment 8

[Igor Bukanov]

(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

[Jeff Walden [:Waldo]]

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

Comment 10

[Igor Bukanov]

(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

[Jesse Ruderman]

I think having an object counter exposed would work for the fuzzer :)

Comment 12

[Igor Bukanov]

Comment on attachment 279611 [details] [diff] [review]
v2

New patch is comming

Comment 13

[Igor Bukanov]

Attachment: v3

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.

Comment 14

[Igor Bukanov]

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

[Jesse Ruderman]

Having a fix for bug 353737 would make it easier to fuzz with evalcx.

Comment 16

[Jesse Ruderman]

Bug 394967, too.

Comment 17

[Jesse Ruderman]

You attached the wrong patch as "v3" here.  That's the patch for bug 394709.

Comment 18

[Igor Bukanov]

Attachment: v3 for real

Here is the right patch.

Comment 19

[Jesse Ruderman]

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.

Comment 20

[Igor Bukanov]

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

Comment 21

[Igor Bukanov]

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.

Comment 22

[Igor Bukanov]

Attachment: v4

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

Comment 23

[Igor Bukanov]

Attachment: v5

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

Comment 24

[Igor Bukanov]

Attachment: v6

The new version uses consistent optional argument formating in help messages.

Comment 25

[Brendan Eich [:brendan]]

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

Comment 26

[Igor Bukanov]

Attachment: v7

Fixing the nits and removing that over engineered show help.

Comment 27

[Igor Bukanov]

Attachment: v8

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.

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 280040 [details] [diff] [review]
v8

Thanks, r+a=me.

/be

Comment 29

[Igor Bukanov]

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