Leak detection helper for js shell

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

11 years ago
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

11 years ago
Created attachment 279574 [details] [diff] [review]
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
Assignee: general → igor
Status: NEW → ASSIGNED
Attachment #279574 - Flags: review?(brendan)
(Assignee)

Comment 2

11 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

11 years ago
Attachment #279574 - Attachment is obsolete: true
(Assignee)

Comment 3

11 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

11 years ago
Summary: Leak detection helper for for js shel → Leak detection helper for js shell
(Assignee)

Comment 4

11 years ago
Created attachment 279611 [details] [diff] [review]
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>
Attachment #279611 - Flags: review?(brendan)

Comment 5

11 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

11 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

11 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

11 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

11 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

11 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

11 years ago
I think having an object counter exposed would work for the fuzzer :)
(Assignee)

Comment 12

11 years ago
Comment on attachment 279611 [details] [diff] [review]
v2

New patch is comming
Attachment #279611 - Flags: review?(brendan)
(Assignee)

Comment 13

11 years ago
Created attachment 279654 [details] [diff] [review]
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.
Attachment #279611 - Attachment is obsolete: true
Attachment #279654 - Flags: review?(brendan)
(Assignee)

Comment 14

11 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

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

Comment 16

11 years ago
Bug 394967, too.

Comment 17

11 years ago
You attached the wrong patch as "v3" here.  That's the patch for bug 394709.
(Assignee)

Comment 18

11 years ago
Created attachment 279703 [details] [diff] [review]
v3 for real

Here is the right patch.
Attachment #279654 - Attachment is obsolete: true
Attachment #279703 - Flags: review?(brendan)
Attachment #279654 - Flags: review?(brendan)

Comment 19

11 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

11 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

11 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

11 years ago
Created attachment 279729 [details] [diff] [review]
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
Attachment #279703 - Attachment is obsolete: true
Attachment #279729 - Flags: review?(brendan)
Attachment #279703 - Flags: review?(brendan)
(Assignee)

Comment 23

11 years ago
Created attachment 279937 [details] [diff] [review]
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().
Attachment #279937 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Attachment #279729 - Attachment is obsolete: true
Attachment #279729 - Flags: review?(brendan)
(Assignee)

Comment 24

11 years ago
Created attachment 279938 [details] [diff] [review]
v6

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

Comment 26

11 years ago
Created attachment 280038 [details] [diff] [review]
v7

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

11 years ago
Created attachment 280040 [details] [diff] [review]
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.
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+
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 395418

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.