807535 - Avoid toggling Ion write barrier too often

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: patch

Toggling JIT write barriers only needs to happen once at the beginning of a GC and then again at the end of the GC. However, while each GC slice is running, we temporarily turn off write barriers in the VM because we don't want to run them while GC code is running.

Right now, every time we turn barriers on or off in the VM, we also change them in the JITs. For slices that aren't the first or the last, this causes us to turn off JIT barriers at the start of each slice and then enable them at the end of the slice. This is pointless because we never actually run any JIT code during a GC.

What we really want to be able to do is disable the barriers in the VM at the start of a slice, but leave them enabled in the JIT. Then, at the end of the slice, we can re-enable the VM barriers and leave the JIT barriers alone. That's what this patch does.

When we just had JM, this didn't matter much because setNeedsBarrier was pretty cheap. But in Ion we iterate over every script. This was costing about 3.5ms for each setNeedsBarrier in my tests. With two calls per slice, that's 7ms of overhead. We only do 10ms of actual GC work per slice, so it's pretty wasteful.

Comment 1

[Sean Stangl [:sstangl]]

Comment on attachment 677234 [details] [diff] [review]
patch

Review of attachment 677234 [details] [diff] [review]:
-----------------------------------------------------------------

Great find. The act of patching the write barriers into Ion code is significantly more expensive on ARM than on x86, so this will be even more beneficent on mobile platforms. Let's try to promote to Aurora once landed.

Comment 2

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/23eb7d58bf90

Comment 3

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

Comment on attachment 677234 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): IonMonkey
User impact if declined: Increased jankiness during GC.
Testing completed (on m-c, etc.): On m-c (soon).
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23eb7d58bf90

Comment 5

[bhavana bajaj [:bajaj]]

Comment on attachment 677234 [details] [diff] [review]
patch

Approving the low risk cleanup patch for ion monkey landing in 18 .

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5bf1d08ae880

Comment 7

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

Please ask first before landing things, Ryan. This had some regressions filed against it yesterday.

Backout:
https://hg.mozilla.org/releases/mozilla-aurora/rev/901aaea897a0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Sorry about that. That said, methinks those regressions should be marked as blocking this bug? This one currently shows no dependencies.

Comment 9

[Andrew McCreight [:mccr8]]

Sorry, that was my fault. I was juggling around the dependencies of some dupes and accidentally dropped the actual dependence for this bug.

Comment 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf7ed9f18a09