Closed Bug 394853 Opened 17 years ago Closed 17 years ago

Leak detection helper for js shell

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 8 obsolete files)

To simplify regression tests for bugs like 394709 it would be nice to have a function is jsshell that returns the current heap size,
Attached patch v1 (obsolete) — Splinter Review
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: general → igor
Status: NEW → ASSIGNED
Attachment #279574 - Flags: review?(brendan)
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)
Attachment #279574 - Attachment is obsolete: true
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
Summary: Leak detection helper for for js shel → Leak detection helper for js shell
Attached patch v2 (obsolete) — Splinter Review
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)
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.
(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?
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?
(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. 
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...)
(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.
I think having an object counter exposed would work for the fuzzer :)
Comment on attachment 279611 [details] [diff] [review]
v2

New patch is comming
Attachment #279611 - Flags: review?(brendan)
Attached patch v3 (obsolete) — Splinter Review
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)
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.
Having a fix for bug 353737 would make it easier to fuzz with evalcx.
Bug 394967, too.
You attached the wrong patch as "v3" here.  That's the patch for bug 394709.
Attached patch v3 for real (obsolete) — Splinter Review
Here is the right patch.
Attachment #279654 - Attachment is obsolete: true
Attachment #279703 - Flags: review?(brendan)
Attachment #279654 - Flags: review?(brendan)
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.
(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.
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.
Attached patch v4 (obsolete) — Splinter Review
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)
Attached patch v5 (obsolete) — Splinter Review
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)
Attachment #279729 - Attachment is obsolete: true
Attachment #279729 - Flags: review?(brendan)
Attached patch v6 (obsolete) — Splinter Review
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 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
Attached patch v7 (obsolete) — Splinter Review
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)
Attached patch v8Splinter Review
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 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+
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 395418
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: