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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Dan Gohman [: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.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

This patch has the same changes to the allocator, except they are gated on a new --ion-regalloc=testbed option.

Comment 3

[Dan Gohman [: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.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47e92bae09fd

Comment 5

[Carsten Book [:Tomcat]]

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