Closed
Bug 863523
Opened 8 years ago
Closed 8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/47ad2545d5f5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Assignee | ||
Updated•8 years ago
|
Attachment #739780 -
Flags: feedback?(bhackett1024)
You need to log in
before you can comment on or make changes to this bug.
Description
•