Closed Bug 807535 Opened 7 years ago Closed 7 years ago

Avoid toggling Ion write barrier too often

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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.
Attachment #677234 - Flags: review?(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.
Attachment #677234 - Flags: review?(sstangl) → review+
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.
Attachment #677234 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/23eb7d58bf90
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 807913
No longer depends on: 807913
Comment on attachment 677234 [details] [diff] [review]
patch

Approving the low risk cleanup patch for ion monkey landing in 18 .
Attachment #677234 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Sorry about that. That said, methinks those regressions should be marked as blocking this bug? This one currently shows no dependencies.
Depends on: 808067
Sorry, that was my fault. I was juggling around the dependencies of some dupes and accidentally dropped the actual dependence for this bug.
You need to log in before you can comment on or make changes to this bug.