624224 - cleanup compartment sweep code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

>     /*
>      * Figure out how much JIT code should be released from inactive compartments.
>      * If multiple eighth-lifes have passed, compound the release interval linearly;

s/lifes/lives/

>      * if enough time has passed, all inactive JIT code will be released.
>      */
>     uint32 releaseInterval = 0;
>     int64 now = PRMJ_Now();
>     if (now >= rt->gcJitReleaseTime) {
>         releaseInterval = 8;
>         while (now >= rt->gcJitReleaseTime) {
>             if (--releaseInterval == 1)
>                 rt->gcJitReleaseTime = now;
>             rt->gcJitReleaseTime += JIT_SCRIPT_EIGHTH_LIFETIME;
>         }
>     }
> 
>+    /* Remove dead wrappers from the compartment map. */
>+    for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); ++c) {
>+        (*c)->sweep(cx, releaseInterval);
>+    }

Nit: don't brace.

>+    

Nit: trailing whitespace and useless blank line.

>     while (read < end) {
>         JSCompartment *compartment = (*read++);

Nit: lose the unnecessary parens.

>         if (compartment->marked) {
>             compartment->marked = false;
>             *write++ = compartment;
>-            /* Remove dead wrappers from the compartment map. */
>-            compartment->sweep(cx, releaseInterval);
>         } else {
>             JS_ASSERT(compartment->freeLists.isEmpty());
>             if (compartment->arenaListsAreEmpty() || gckind == GC_LAST_CONTEXT) {

gckind condition is loop invariant -- specialize by hoisting?

>                 if (callback)
>                     (void) callback(cx, compartment, JSCOMPARTMENT_DESTROY);
>                 if (compartment->principals)
>                     JSPRINCIPALS_DROP(cx, compartment->principals);
>                 delete compartment;
>             } else {
>                 compartment->marked = false;
>                 *write++ = compartment;
>-                compartment->sweep(cx, releaseInterval);

Repeats the first if's consequent. Could make this common code at the bottom of
the loop body, continue past it if (!compartment->marked &&
compartment->arenaListsAreEmpty()) after doing the callback and delete. This
raises the question: if the compartment is marked but its arenaListsAreEmpty(),
isn't something wrong? How could that happen?

The answer may be that other code is simpler if an empty compartment can be
marked, but if not, then the condition guarding the early
callback/DROP/delete/continue if-then is even simpler.

>     /*
>      * We finalize iterators before other objects so the iterator can use the
>      * object which properties it enumerates over to finalize the enumeration
>      * state. We finalize objects before other GC things to ensure that
>      * object's finalizer can access them even if they will be freed.
>      */
> 

Lose this blank line.

>     for (JSCompartment **comp = rt->compartments.begin(); comp != rt->compartments.end(); comp++)

It seems |c| is the canonical name for JSCompartment ** variables, suggest
using it consistently. It pays off for readers and grep.

/be

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> Repeats the first if's consequent. Could make this common code at the bottom of
> the loop body, continue past it if (!compartment->marked &&
> compartment->arenaListsAreEmpty()) after doing the callback and delete. This
> raises the question: if the compartment is marked but its arenaListsAreEmpty(),
> isn't something wrong? How could that happen?
> 
> The answer may be that other code is simpler if an empty compartment can be
> marked, but if not, then the condition guarding the early
> callback/DROP/delete/continue if-then is even simpler.

And we should assert the that !compartment->marked if we can avoid testing it.

/be

Comment 2

[Gregor Wagner [:gwagner]]

(In reply to comment #0)
> Repeats the first if's consequent. Could make this common code at the bottom of
> the loop body, continue past it if (!compartment->marked &&
> compartment->arenaListsAreEmpty()) after doing the callback and delete. This
> raises the question: if the compartment is marked but its arenaListsAreEmpty(),
> isn't something wrong? How could that happen?

This can happen if we are creating a compartment and the allocation of the global object triggers a GC (gczeal for example). We had this bug before.

Comment 3

[Gregor Wagner [:gwagner]]

Attachment: v1

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 503644 [details] [diff] [review]
patch

>+        if (!compartment->marked && compartment->arenaListsAreEmpty() || gckind == GC_LAST_CONTEXT) {

Parenthesize && against || even though formally unnecessary, or GCC will whine.

Also a comment about the && condition would be helpful.

r=me with these, thanks.

/be

Comment 6

[Gregor Wagner [:gwagner]]

http://hg.mozilla.org/tracemonkey/rev/1548012fa682

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 503644 [details] [diff] [review]
patch

>-    /* Delete atomsCompartment only during runtime shutdown */
>-    rt->atomsCompartment->marked = true;
>-

Why was this deleted? Its lack seems to cause leaks. Nick noticed and is backing out. Sorry I missed this.

/be

Comment 8

[Nicholas Nethercote [inactive]]

Backout:
http://hg.mozilla.org/tracemonkey/rev/7c578c10675d

Comment 9

[Nicholas Nethercote [inactive]]

Here are the leaks, in case it's helpful:

==8316== Memcheck, a memory error detector
==8316== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==8316== Using Valgrind-3.7.0.SVN and LibVEX; rerun with -h for copyright info
==8316== Command: debug32/js -j -m -p /home/njn/moz/SunSpider/tests/sunspider-0.9.1//3d-cube.js
==8316== 
==8316== 
==8316== HEAP SUMMARY:
==8316==     in use at exit: 931,440 bytes in 28 blocks
==8316==   total heap usage: 1,718 allocs, 1,690 frees, 3,858,710 bytes allocated
==8316== 
==8316== 24,576 bytes in 1 blocks are possibly lost in loss record 22 of 28
==8316==    at 0x48D2230: malloc (vg_replace_malloc.c:236)
==8316==    by 0x805B5F0: js_malloc (jsutil.h:209)
==8316==    by 0x8077F2A: js::SystemAllocPolicy::malloc(unsigned int) (jstl.h:255)
==8316==    by 0x823E8A6: js::detail::HashTable<js::HashMap<unsigned char*, unsigned int, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned int, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::createTable(js::SystemAllocPolicy&, unsigned int) (jshashtable.h:295)
==8316==    by 0x823C92B: js::detail::HashTable<js::HashMap<unsigned char*, unsigned int, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::Entry, js::HashMap<unsigned char*, unsigned int, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::init(unsigned int) (jshashtable.h:351)
==8316==    by 0x823A7CD: js::HashMap<unsigned char*, unsigned int, js::DefaultHasher<unsigned char*>, js::SystemAllocPolicy>::init(unsigned int) (jshashtable.h:810)
==8316==    by 0x8206E2D: js::InitJIT(js::TraceMonitor*) (jstracer.cpp:7653)
==8316==    by 0x80A837D: JSCompartment::init() (jscompartment.cpp:112)
==8316==    by 0x805F7AF: JSRuntime::init(unsigned int) (jsapi.cpp:641)
==8316==    by 0x806B989: JS_Init (jsapi.cpp:752)
==8316==    by 0x8056F1F: main (js.cpp:5538)
==8316== 
==8316== 262,164 bytes in 1 blocks are possibly lost in loss record 25 of 28
==8316==    at 0x48D2A0E: operator new(unsigned int) (vg_replace_malloc.c:255)
==8316==    by 0x8206EBD: js::InitJIT(js::TraceMonitor*) (jstracer.cpp:7663)
==8316==    by 0x80A837D: JSCompartment::init() (jscompartment.cpp:112)
==8316==    by 0x805F7AF: JSRuntime::init(unsigned int) (jsapi.cpp:641)
==8316==    by 0x806B989: JS_Init (jsapi.cpp:752)
==8316==    by 0x8056F1F: main (js.cpp:5538)
==8316== 
==8316== 644,700 (4,344 direct, 640,356 indirect) bytes in 1 blocks are definitely lost in loss record 28 of 28
==8316==    at 0x48D2A0E: operator new(unsigned int) (vg_replace_malloc.c:255)
==8316==    by 0x805F77F: JSRuntime::init(unsigned int) (jsapi.cpp:641)
==8316==    by 0x806B989: JS_Init (jsapi.cpp:752)
==8316==    by 0x8056F1F: main (js.cpp:5538)
==8316== 
==8316== LEAK SUMMARY:
==8316==    definitely lost: 4,344 bytes in 1 blocks
==8316==    indirectly lost: 640,356 bytes in 25 blocks
==8316==      possibly lost: 286,740 bytes in 2 blocks
==8316==    still reachable: 0 bytes in 0 blocks
==8316==         suppressed: 0 bytes in 0 blocks
==8316== 
==8316== For counts of detected and suppressed errors, rerun with: -v
==8316== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 21 from 6)

Comment 10

[Gregor Wagner [:gwagner]]

Attachment: patch

Uh bad one from my side. 
The atomsCompartment gets removed from the compartments vector right away... Sorry!

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

Please remove the fixed-in-tracemonkey tag when things are backed out.

http://hg.mozilla.org/mozilla-central/rev/1548012fa682

Comment 12

[Gregor Wagner [:gwagner]]

Comment on attachment 503738 [details] [diff] [review]
patch

Yeah never underestimate cleanup patches.
The problem was that the atomsCompartment didn't get returned into the compartments vector.
This should take care of it now:
+            if (compartment == rt->atomsCompartment) {
+                *write++ = compartment;
+                continue;
+            }

Marking the atomsCompartment is not required any more because we don't want to delete it even if we are performing LAST_CONTEXT GC. 
We could mark it and add this check in the if with gckind == LAST_CONTEXT && comp != rt->atomsCompartment. Either way works for me.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 503738 [details] [diff] [review]
patch

>     while (read < end) {
>+        JSCompartment *compartment = *read++;
>+        /* Unmarked compartments containing marked objects don't get deleted, except LAST_CONTEXT GC is performed. */

Blank line before entire-line-or-greater comment unless preceding non-space char is {.

>+        if ((!compartment->marked && compartment->arenaListsAreEmpty()) || gckind == GC_LAST_CONTEXT) {
>+            JS_ASSERT(compartment->freeLists.isEmpty());
>+            /* AtomsCompartment gets destroyed in js_FinishGC. */

Ditto.

>+            if (compartment == rt->atomsCompartment) {
>+                *write++ = compartment;
>+                continue;
>+            }

Could we avoid putting atomsCompartment in the vector, or always put it at [0] and skip it?

>+            if (callback)
>+                (void) callback(cx, compartment, JSCOMPARTMENT_DESTROY);
>+            if (compartment->principals)
>+                JSPRINCIPALS_DROP(cx, compartment->principals);
>+            delete compartment;

I still prefer continue here and no else, especially with the continue you added.

r=me but avoid the added branch if you can.

/be

Comment 14

[Gregor Wagner [:gwagner]]

Attachment: patch

You are right, AtomsCompartment == compartments[0] should always hold.
The else part is not necessary any more.
Thx!

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 503973 [details] [diff] [review]
patch

>     JSCompartment **read = rt->compartments.begin();
>     JSCompartment **end = rt->compartments.end();
>     JSCompartment **write = read;
> 
>+    /* Skip the atomsCompartment */
>+    JS_ASSERT(rt->compartments.length() >= 1);
>+    JS_ASSERT(*read == rt->atomsCompartment);    
>+    *write++ = *read++;

No need to copy if you trust the asserted conditions (we generally do, esp. in GC; we have runtime checks only in the decompiler, as a rule). Could even avoid two ++ operations if you set read and write carefully and adjust the assertions.

/be

Comment 16

[Gregor Wagner [:gwagner]]

Attachment: patch

Finally :)

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 504024 [details] [diff] [review]
patch

r+ carry-over, please obsolete old patch when attaching new one.

/be

Comment 18

[Gregor Wagner [:gwagner]]

http://hg.mozilla.org/tracemonkey/rev/a13615394e09

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a13615394e09