Closed
Bug 624224
Opened 14 years ago
Closed 14 years ago
cleanup compartment sweep code
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: gal, Assigned: gwagner)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 4 obsolete files)
5.24 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
> /*
> * 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•14 years ago
|
Assignee: general → anygregor
Comment 1•14 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?
>
> 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•14 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•14 years ago
|
||
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #502543 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #503644 -
Flags: review?(brendan)
Comment 5•14 years ago
|
||
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•14 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 7•14 years ago
|
||
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-
![]() |
||
Comment 8•14 years ago
|
||
![]() |
||
Comment 9•14 years ago
|
||
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•14 years ago
|
||
Uh bad one from my side.
The atomsCompartment gets removed from the compartments vector right away... Sorry!
Attachment #503644 -
Attachment is obsolete: true
Comment 11•14 years ago
|
||
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•14 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 13•14 years ago
|
||
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•14 years ago
|
||
You are right, AtomsCompartment == compartments[0] should always hold.
The else part is not necessary any more.
Thx!
Comment 15•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #503973 -
Attachment is obsolete: true
Comment 17•14 years ago
|
||
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•14 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 19•14 years ago
|
||
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a13615394e09
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•