Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2392333002:
[ignition/turbo] Remove stack check from inlined functions (Closed)
|
Created:
4 years, 2 months ago by Leszek Swirski Modified:
4 years, 1 month ago CC:
v8-reviews_googlegroups.com, rmcilroy Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[ignition/turbo] Remove stack check from inlined functions
This removes the first stack check in inlined functions in the bytecode
graph builder, to match the behaviour of the AST graph builder.
I measure a ~1% statistically significant (p < 0.01) improvement on
Mandreel with --ignition-staging --turbo (on my x64 machine, YMMV).
Committed: https://crrev.com/cf1ebf36625ce8ffa713822768acfa59153440fc
Cr-Commit-Position: refs/heads/master@{#40715}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Only remove the first stack check in functions #Patch Set 4 : Rebase #Patch Set 5 : Rebase #
Total comments: 2
Patch Set 6 : Change 'while' to 'for' #
Messages
Total messages: 33 (24 generated)
The CQ bit was checked by leszeks@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/10151) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
The CQ bit was checked by leszeks@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [ignition/turbo] Remove stack checks from inlined functions This matches the behaviour of the AST graph builder. ========== to ========== [ignition/turbo] Remove stack check from inlined functions This removes the first stack check in inlined functions in the bytecode graph builder, to match the behaviour of the AST graph builder. I measure a ~1% improvement on Mandreel with --ignition-staging --turbo (on my x64 machine, YMMV). ==========
leszeks@chromium.org changed reviewers: + bmeurer@chromium.org, mstarzinger@chromium.org
Hi Turbofan folks, As discussed in the standup, here's the hack to remove stack checks from inlined functions. It doesn't do much to close the ast/bytecode graph builder gap, but no one ever got fired for decreasing cycles.
Description was changed from ========== [ignition/turbo] Remove stack check from inlined functions This removes the first stack check in inlined functions in the bytecode graph builder, to match the behaviour of the AST graph builder. I measure a ~1% improvement on Mandreel with --ignition-staging --turbo (on my x64 machine, YMMV). ========== to ========== [ignition/turbo] Remove stack check from inlined functions This removes the first stack check in inlined functions in the bytecode graph builder, to match the behaviour of the AST graph builder. I measure a ~1% statistically significant (p < 0.01) improvement on Mandreel with --ignition-staging --turbo (on my x64 machine, YMMV). ==========
The CQ bit was checked by leszeks@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Michi, how do you feel about this then? Should we land it or abandon it? I've measured that ~1% improvement on a few other benchmarks now.
mstarzinger@chromium.org changed required reviewers: + bmeurer@chromium.org
LGTM on the implementation, just one suggestion. I would have hoped for a bigger payoff performance-wise from this change. Benedikt, do you think we should land this? https://codereview.chromium.org/2392333002/diff/80001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2392333002/diff/80001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:723: while (!iterator.done()) { suggestion: Could we rewrite this while-loop as a for-loop (with {iterator.Advance()} as part of the loop)? Then the below special case could be expressed as a simple "continue" instead of having to indent the whole switch even further. Just aesthetics, no strong opinion.
The CQ bit was checked by leszeks@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Michi, I've addressed your comment. For the performance improvement comment, I'll run a couple hundred iterations of full octane overnight to see how all the benchmarks are affected (on my machine at least), my previous comments were just based off one or two of them. https://codereview.chromium.org/2392333002/diff/80001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/2392333002/diff/80001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:723: while (!iterator.done()) { On 2016/10/31 14:39:01, Michael Starzinger wrote: > suggestion: Could we rewrite this while-loop as a for-loop (with > {iterator.Advance()} as part of the loop)? Then the below special case could be > expressed as a simple "continue" instead of having to indent the whole switch > even further. Just aesthetics, no strong opinion. I like it! Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
After 700ish runs on my machine, looks like improvements are better than I'd thought: Box2D Δmean: 4.34%, Δmedian: 4.79% (p = 7.85e-07) DeltaBlue Δmean: 5.65%, Δmedian: 5.53% (p = 9.39e-49) EarleyBoyer Δmean: 5.13%, Δmedian: 4.69% (p = 2.66e-39) Gameboy Δmean: 4.65%, Δmedian: 2.17% (p = 0.00162) Mandreel Δmean: 2.74%, Δmedian: 1.66% (p = 1.83e-07) MandreelLatency Δmean: 2.04%, Δmedian: 1.48% (p = 0.00103) PdfJS Δmean: 1.71%, Δmedian: 1.46% (p = 0.00323) Richards Δmean: 2.69%, Δmedian: 2.69% (p = 1.2e-30) Typescript Δmean: 0.879%, Δmedian: 1.02% (p = 0.00421) p-values should be taken with a grain of salt because these aren't really normal distributions (plus, we're running it several times over the data, so we're likely to hit the multiple comparison problem, a.k.a. https://xkcd.com/882/)
LGTM, I'd be fine with landing this.
The CQ bit was checked by leszeks@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2392333002/#ps100001 (title: "Change 'while' to 'for'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [ignition/turbo] Remove stack check from inlined functions This removes the first stack check in inlined functions in the bytecode graph builder, to match the behaviour of the AST graph builder. I measure a ~1% statistically significant (p < 0.01) improvement on Mandreel with --ignition-staging --turbo (on my x64 machine, YMMV). ========== to ========== [ignition/turbo] Remove stack check from inlined functions This removes the first stack check in inlined functions in the bytecode graph builder, to match the behaviour of the AST graph builder. I measure a ~1% statistically significant (p < 0.01) improvement on Mandreel with --ignition-staging --turbo (on my x64 machine, YMMV). ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [ignition/turbo] Remove stack check from inlined functions This removes the first stack check in inlined functions in the bytecode graph builder, to match the behaviour of the AST graph builder. I measure a ~1% statistically significant (p < 0.01) improvement on Mandreel with --ignition-staging --turbo (on my x64 machine, YMMV). ========== to ========== [ignition/turbo] Remove stack check from inlined functions This removes the first stack check in inlined functions in the bytecode graph builder, to match the behaviour of the AST graph builder. I measure a ~1% statistically significant (p < 0.01) improvement on Mandreel with --ignition-staging --turbo (on my x64 machine, YMMV). Committed: https://crrev.com/cf1ebf36625ce8ffa713822768acfa59153440fc Cr-Commit-Position: refs/heads/master@{#40715} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cf1ebf36625ce8ffa713822768acfa59153440fc Cr-Commit-Position: refs/heads/master@{#40715} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
