Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667573002:
[Turbofan] don't optimize from bytecode > 250K in size. (Closed)
|
Created:
3 years, 10 months ago by mvstanton Modified:
3 years, 10 months ago Reviewers:
Michael Starzinger CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Turbofan] don't optimize from bytecode > 250K in size.
Compiles simply take too long, and such functions are liable to deopt
quickly.
BUG=chromium:686153
Review-Url: https://codereview.chromium.org/2667573002
Cr-Commit-Position: refs/heads/master@{#42779}
Committed: https://chromium.googlesource.com/v8/v8/+/475b455b6f67cc825d02641499ab4558f83d6d9a
Patch Set 1 #
Total comments: 5
Patch Set 2 : comments #Messages
Total messages: 16 (9 generated)
mvstanton@chromium.org changed reviewers: + mstarzinger@chromium.org
Hi Michael, Here is the issue we discussed before lunch. I'd like to port this to M56/57 as well... Thanks, --Michael
The CQ bit was checked by mvstanton@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...
https://codereview.chromium.org/2667573002/diff/1/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://codereview.chromium.org/2667573002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:61: static const int kMaxSizeOptFromBytecode = 250 * 1024; nit: Can we call this "kMaxSizeOptIgnition" to follow the same naming convention as the above constants. Also, it might make sense to mention that the only reason this is not based on kCodeSizeMultiplier is because there is no such heuristic for optimization of baseline frames (i.e. no kMaxSizeOpt). https://codereview.chromium.org/2667573002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:400: if (shared->bytecode_array()->Size() > kMaxSizeOptFromBytecode) { nit: s/Size/instruction_size/ to avoid dispatched getter and for readability and to keep it in sync with the other heuristics in this file that are based on "bytecode" size.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM modulo nits.
https://codereview.chromium.org/2667573002/diff/1/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://codereview.chromium.org/2667573002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:400: if (shared->bytecode_array()->Size() > kMaxSizeOptFromBytecode) { On 2017/01/30 12:42:11, Michael Starzinger wrote: > nit: s/Size/instruction_size/ to avoid dispatched getter and for readability and > to keep it in sync with the other heuristics in this file that are based on > "bytecode" size. Ooops, looked at the wrong use-site. Please disregard. Sorry for the noise.
Thanks Michael, landing... --Michael https://codereview.chromium.org/2667573002/diff/1/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://codereview.chromium.org/2667573002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:61: static const int kMaxSizeOptFromBytecode = 250 * 1024; On 2017/01/30 12:42:11, Michael Starzinger wrote: > nit: Can we call this "kMaxSizeOptIgnition" to follow the same naming convention > as the above constants. Also, it might make sense to mention that the only > reason this is not based on kCodeSizeMultiplier is because there is no such > heuristic for optimization of baseline frames (i.e. no kMaxSizeOpt). "We are not using the code size multiplier here because there is no "kMaxSizeOpt" with which we would need to normalize." Better name: kMaxSizeOptIgnition. Done. https://codereview.chromium.org/2667573002/diff/1/src/runtime-profiler.cc#new... src/runtime-profiler.cc:400: if (shared->bytecode_array()->Size() > kMaxSizeOptFromBytecode) { On 2017/01/30 12:42:11, Michael Starzinger wrote: > nit: s/Size/instruction_size/ to avoid dispatched getter and for readability and > to keep it in sync with the other heuristics in this file that are based on > "bytecode" size. Per discussion, Size() is now the right predicate.
The CQ bit was checked by mvstanton@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/2667573002/#ps20001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485788185778780,
"parent_rev": "626b95e37e58d10e9f504a645163c173b938fd21", "commit_rev":
"475b455b6f67cc825d02641499ab4558f83d6d9a"}
Message was sent while issue was closed.
Description was changed from ========== [Turbofan] don't optimize from bytecode > 250K in size. Compiles simply take too long, and such functions are liable to deopt quickly. BUG=chromium:686153 ========== to ========== [Turbofan] don't optimize from bytecode > 250K in size. Compiles simply take too long, and such functions are liable to deopt quickly. BUG=chromium:686153 Review-Url: https://codereview.chromium.org/2667573002 Cr-Commit-Position: refs/heads/master@{#42779} Committed: https://chromium.googlesource.com/v8/v8/+/475b455b6f67cc825d02641499ab4558f83... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/475b455b6f67cc825d02641499ab4558f83... |
