Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: gal, Assigned: gwagner)

Tracking

Trunk
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

9 years ago
>     /*
>      * 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
Reporter

Updated

9 years ago
Assignee: general → anygregor
(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
Assignee

Comment 2

9 years ago
(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.
Assignee

Comment 3

9 years ago
Posted patch v1 (obsolete) — Splinter Review
Assignee

Comment 4

9 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #502543 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Attachment #503644 - Flags: review?(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
Attachment #503644 - Flags: review?(brendan) → review+
Assignee

Comment 6

9 years ago
http://hg.mozilla.org/tracemonkey/rev/1548012fa682
Whiteboard: fixed-in-tracemonkey
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
Attachment #503644 - Flags: review+ → review-
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)
Assignee

Comment 10

9 years ago
Posted patch patch (obsolete) — Splinter Review
Uh bad one from my side. 
The atomsCompartment gets removed from the compartments vector right away... Sorry!
Attachment #503644 - Attachment is obsolete: true
Please remove the fixed-in-tracemonkey tag when things are backed out.

http://hg.mozilla.org/mozilla-central/rev/1548012fa682
Whiteboard: fixed-in-tracemonkey
Assignee

Comment 12

9 years ago
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.
Attachment #503738 - Flags: review?(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
Attachment #503738 - Flags: review?(brendan) → review+
Assignee

Comment 14

9 years ago
Posted patch patch (obsolete) — Splinter Review
You are right, AtomsCompartment == compartments[0] should always hold.
The else part is not necessary any more.
Thx!
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
Attachment #503973 - Flags: review+
Assignee

Comment 16

9 years ago
Posted patch patchSplinter Review
Finally :)
Attachment #503738 - Attachment is obsolete: true
Attachment #503973 - Attachment is obsolete: true
Comment on attachment 504024 [details] [diff] [review]
patch

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

/be
Attachment #504024 - Flags: review+
Assignee

Comment 18

9 years ago
http://hg.mozilla.org/tracemonkey/rev/a13615394e09
Whiteboard: fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.