863523 - Unconditionally mark all global objects when doing a minor collection

Status: RESOLVED FIXED

Description

[Terrence Cole [:terrence]]

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.

Comment 1

[Terrence Cole [:terrence]]

Created attachment 739780 [details] [diff] [review]
v0

I thought I had uploaded this last night. Oh well.

Comment 2

[Terrence Cole [:terrence]]

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 4

[Terrence Cole [:terrence]]

(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?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Terrence Cole [:terrence]]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Terrence Cole [:terrence]]

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

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/47ad2545d5f5