Closed Bug 1170840 Opened 4 years ago Closed 4 years ago

Don't try to distinguish pre/post cold code when splitting bundles based on hot code

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
If the backtracking allocator decides to split a bundle which contains both hot and cold code, it can end up creating three bundles --- one with the hot code, one with cold code before the hot code, and one with cold code after the hot code.  If the script can directly jump from the earlier cold bundle to the later cold bundle (say we split around a loop in a conditional 'if (...) { while (...) {} }' and have cold code before and after the 'if'), then since the bundles are allocated independently this can introduce a lot of moves.

This wouldn't be much of a problem if we could accurately determine which code is hot and cold, but we can't.  If the cold code is actually pretty hot compared to the 'hot' code then performance is badly affected.  This shows up on asmjs-apps-zlib, where a loop in a conditional is actually pretty cold compared to the code before/after the conditional.  The attached patch uses a single bundle for cold code, which improves this benchmark.  It's possible this change could hurt allocation in other tests, but I think this would be better dealt with by having a separate splitting strategy that breaks up discontinuous bundles, rather than adding more complexity here.
Attachment #8614418 - Flags: review?(sunfish)
Comment on attachment 8614418 [details] [diff] [review]
patch

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

This approach looks nice, because in theory the combined cold bundle could always be split later, if that turns out to be needed.
Attachment #8614418 - Flags: review?(sunfish) → review+
Attached patch patchSplinter Review
This patch has the same changes to the allocator, except they are gated on a new --ion-regalloc=testbed option.
Assignee: nobody → bhackett1024
Attachment #8614418 - Attachment is obsolete: true
Attachment #8616400 - Flags: review?(sunfish)
Comment on attachment 8616400 [details] [diff] [review]
patch

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

Looks like everything is safely under the testbad flag. LGTM.
Attachment #8616400 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/47e92bae09fd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.