Closed
Bug 863523
Opened 12 years ago
Closed 12 years ago
Unconditionally mark all global objects when doing a minor collection
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file)
2.33 KB,
patch
|
billm
:
review+
terrence
:
review+
|
Details | Diff | Splinter Review |
Brian's research found that marking all globals during minor GC is less expensive than taking a store buffer update for every write to a global.
Assignee | ||
Comment 1•12 years ago
|
||
I thought I had uploaded this last night. Oh well.
Attachment #739780 -
Flags: review?(wmccloskey)
Attachment #739780 -
Flags: review?(terrence)
Assignee | ||
Comment 2•12 years ago
|
||
Comment on attachment 739780 [details] [diff] [review]
v0
Review of attachment 739780 [details] [diff] [review]:
-----------------------------------------------------------------
It occurred to me that I am effectively reviewing these hunks as I split them apart from Brian's mega-patch. Note: For the patches that are a direct copy of a hunk from the mega-patch, I plan to land them with the header from the mega-patch so that the attribution remains correct.
Attachment #739780 -
Flags: review?(terrence)
Attachment #739780 -
Flags: review+
Attachment #739780 -
Flags: feedback?(bhackett1024)
Comment on attachment 739780 [details] [diff] [review]
v0
Review of attachment 739780 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/gc/RootMarking.cpp
@@ +664,5 @@
> + * needing a write barrier.
> + */
> + JS_ASSERT(trc->runtime->isHeapMinorCollecting());
> +
> + if (!compartment->ionCompartment())
This check seems like one that people are liable to forget is there, causing us to skip barriers when we shouldn't. Is it important for performance?
Attachment #739780 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #3)
>
> This check seems like one that people are liable to forget is there, causing
> us to skip barriers when we shouldn't. Is it important for performance?
I don't actually know. What situation are you seeing where we could forget to mark global slots? Writes from the interpreter are going to go through HeapSlot and I thought the jits were not allowed to make non-own-global accesses directly. Maybe I'm wrong about the second assumption though?
I'm worried that people will say, "Global slots don't need post-barriers," and then remove them from the interpreter on an ad-hoc basis, forgetting that we still need them for the interpreter. I guess as long as we have tests, it's not really a problem.
Assignee | ||
Comment 6•12 years ago
|
||
Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=0ccf9f11d4d9
Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0789f4c3113
Comment 7•12 years ago
|
||
Shockingly, Android No-Ion builds didn't care for this. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/320d1b78f963
https://tbpl.mozilla.org/php/getParsedLog.php?id=22426064&tree=Mozilla-Inbound
../../../js/src/gc/RootMarking.cpp: In function 'void MarkGlobalForMinorGC(JSTracer*, JSCompartment*)':
../../../js/src/gc/RootMarking.cpp:666:23: error: 'JSCompartment' has no member named 'ionCompartment'
make[5]: *** [RootMarking.o] Error 1
Assignee | ||
Comment 8•12 years ago
|
||
Indeed, it was missing the JS_ION guards. Tested to make sure that was all at:
https://tbpl.mozilla.org/?tree=Try&rev=e9bea27d891c
Repushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47ad2545d5f5
Comment 9•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Assignee | ||
Updated•12 years ago
|
Attachment #739780 -
Flags: feedback?(bhackett1024)
You need to log in
before you can comment on or make changes to this bug.
Description
•