Unconditionally mark all global objects when doing a minor collection

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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

6 years ago
Created attachment 739780 [details] [diff] [review]
v0

I thought I had uploaded this last night. Oh well.
Attachment #739780 - Flags: review?(wmccloskey)
Attachment #739780 - Flags: review?(terrence)
(Assignee)

Comment 2

6 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

6 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.
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

6 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
https://hg.mozilla.org/mozilla-central/rev/47ad2545d5f5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Updated

6 years ago
Attachment #739780 - Flags: feedback?(bhackett1024)
You need to log in before you can comment on or make changes to this bug.