487191 - Lk increase due to tracemonkey merge on April 7th

Status: RESOLVED DUPLICATE of bug 487967

(Core :: JavaScript Engine, defect, P1)

Description

[Mark Banner (:standard8)]

Attachment: Leak diff extract

The recent tracemonkey merge on April 7th caused Lk regressions on all Firefox tier-1 platforms. The checkins:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-04-07+00%3A07&enddate=2009-04-07+00%3A08

Linux: Approx 3kB increase.
Mac: Hidden due to current size of leak.
Windows: Approx 4kB increase.

Linux Log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239088257.1239089374.2927.gz

Leak diff extract attached.

Comment 1

[Robert Sayre]

Yep, some of the new stuff is leaking.

valgrind --leak-check=full ./js -j -e "gSkipSlowTests=true" -f trace-test.js

http://pastebin.mozilla.org/641166

Comment 2

[Brendan Eich [:brendan]]

I ran with this cmd line in gdb:

r -e 'dumpHeap("/tmp/before.heap")' -j -e "gSkipSlowTests=true" -f trace-test.js -f cleanup.js -e 'gc(), dumpHeap("/tmp/after.heap")'

cleanup.js is just:

for (i in this)
    delete this[i];
delete i;

The only patch is to jscntxt.cpp:

diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
--- a/js/src/jscntxt.cpp
+++ b/js/src/jscntxt.cpp
@@ -666,17 +666,20 @@ js_DestroyContext(JSContext *cx, JSDestr
          * request to end.  We'll let it run below, just before we do the truly
          * final GC and then free atom state.
          */
         while (cx->requestDepth != 0)
             JS_EndRequest(cx);
 #endif
 
         if (last) {
+            memset(rt->builtinFunctions, 0, sizeof rt->builtinFunctions);
+
             js_GC(cx, GC_LAST_CONTEXT);
+{ FILE *fp = fopen("/tmp/final.heap", "w"); if (fp) { JS_DumpHeap(cx, fp, NULL, 0, NULL, (size_t)-1, NULL); fclose(fp); } }
             DUMP_EVAL_CACHE_METER(cx);
             DUMP_FUNCTION_METER(cx);
 
             /*
              * Free the script filename table if it exists and is empty. Do this
              * after the last GC to avoid finalizers tripping on free memory.
              */
             if (rt->scriptFilenameTable &&

Without the memset to clear builtin functions, I see final.heap showing some leaks. With that memset, final.heap is a zero-length file.

Did some recent checkin fix this bug, or am I not doing the right thing to reproduce?

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: clearn rt->builtinFunctions before last GC

Comment 4

[Brendan Eich [:brendan]]

I asked about recent checkins cuz the final.heap contents were only about 8 to 10 lines without the memset. Not enough to account for the valgrind log.

/be

Comment 5

[Brendan Eich [:brendan]]

Wow, dbaron nailed this at fifty paces. Dup'ing forward since he has the patch and diagnosis. Thanks, David!

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 372166 [details] [diff] [review]
clearn rt->builtinFunctions before last GC

Over in bug 487968 now.

/be