Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2619773002:
[turbofan] Use feedback from StoreDataPropertyInLiteral. (Closed)
|
Created:
3 years, 11 months ago by Franzi Modified:
3 years, 11 months ago Reviewers:
Benedikt Meurer CC:
v8-reviews_googlegroups.com, rmcilroy Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Use feedback from StoreDataPropertyInLiteral.
Lower StoreDataPropertyInLiteral() when storing
computed property names in object literals.
Add a new AccessMode, kStoreInLiteral. It is similar to
AccessMode::kStore but does not look
up properties on the prototype chain.
99% of all literal definitions with computed property names
end up with generic access_info because of how we count
properties. Once we fix
https://bugs.chromium.org/p/v8/issues/detail?id=5625,
they'll get lowered as well.
BUG=v8:5624
Review-Url: https://codereview.chromium.org/2619773002
Cr-Commit-Position: refs/heads/master@{#42210}
Committed: https://chromium.googlesource.com/v8/v8/+/088df4e13805b5a41bca3ed19030ff7c0c6f2df0
Patch Set 1 #Patch Set 2 : Add AccessMode for storing in literals. #Patch Set 3 : Rebase. #Patch Set 4 : Fix merge messups. #Patch Set 5 : Delete comment. #
Total comments: 8
Patch Set 6 : Address review comments. #
Total comments: 6
Patch Set 7 : Fix DCHECKs. #
Messages
Total messages: 38 (31 generated)
The CQ bit was checked by franzih@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_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/18749) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/18812) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/18719) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/31847)
The CQ bit was checked by franzih@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 checked by franzih@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...
Description was changed from ========== [turbofan] WIP: Use feedback from StoreDataPropInLiteral. BUG= ========== to ========== [turbofan] Use feedback from StoreDataPropertyInLiteral. Lower StoreDataPropertyInLiteral() when storing computed property names in object literals. Add a new AccessMode, kStoreInLiteral. It is similar to AccessMode::kStore but does not look up properties on the prototype chain. 99% of all literal definitions with computed property names end up with generic access_info because of how we count properties. Once we fix https://bugs.chromium.org/p/v8/issues/detail?id=5625, they'll get lowered as well. BUG=v8:5624 ==========
franzih@chromium.org changed reviewers: + bmeurer@chromium.org
Hi Benedikt, Here's the CL that's using the feedback collected in runtime_StoreDataPropertyInLiteral. I've added an AccessMode::kStoreInLiteral so we don't look up the prototype chain. Please take a look. Thanks, Franzi
The CQ bit was checked by franzih@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.
https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... src/compiler/js-native-context-specialization.cc:970: if (access_info.holder().ToHandle(&holder) && There should never be a holder for kStoreInLiteral. So this should rather be a DCHECK. https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... src/compiler/js-native-context-specialization.cc:1297: DCHECK_EQ(1, access_info.receiver_maps().size()); This check doesn't need to be here. https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... src/compiler/js-native-context-specialization.cc:1310: effect = BuildCheckMaps(receiver, effect, control, MapList{receiver_map}); You can use access_info.receiver_maps() here instead of MapList{receiver_map}. https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... src/compiler/js-native-context-specialization.cc:1511: access_mode == AccessMode::kStoreInLiteral) && This doesn't make sense. We never do a store in literal to elements.
The CQ bit was checked by franzih@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.
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by franzih@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/2619773002/diff/80001/src/compiler/js-native-... File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... src/compiler/js-native-context-specialization.cc:970: if (access_info.holder().ToHandle(&holder) && On 2017/01/10 08:25:55, Benedikt Meurer wrote: > There should never be a holder for kStoreInLiteral. So this should rather be a > DCHECK. OK. Done. https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... src/compiler/js-native-context-specialization.cc:1297: DCHECK_EQ(1, access_info.receiver_maps().size()); On 2017/01/10 08:25:55, Benedikt Meurer wrote: > This check doesn't need to be here. Oops, left-over debugging stuff. Deleted. https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... src/compiler/js-native-context-specialization.cc:1310: effect = BuildCheckMaps(receiver, effect, control, MapList{receiver_map}); On 2017/01/10 08:25:54, Benedikt Meurer wrote: > You can use access_info.receiver_maps() here instead of MapList{receiver_map}. Done. Changed the other uses of MapList{} as well. https://codereview.chromium.org/2619773002/diff/80001/src/compiler/js-native-... src/compiler/js-native-context-specialization.cc:1511: access_mode == AccessMode::kStoreInLiteral) && On 2017/01/10 08:25:54, Benedikt Meurer wrote: > This doesn't make sense. We never do a store in literal to elements. OK, also added a DCHECK to beginning of the function
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % nits, thanks! https://codereview.chromium.org/2619773002/diff/120001/src/compiler/js-native... File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2619773002/diff/120001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:971: DCHECK_NE(access_mode, AccessMode::kStoreInLiteral); Nit: The other way around, it's DCHECK_NE(unexpected, actual) https://codereview.chromium.org/2619773002/diff/120001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:1357: DCHECK_NE(access_mode, AccessMode::kStoreInLiteral); Nit: Same as above. https://codereview.chromium.org/2619773002/diff/120001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:1455: case AccessMode::kStoreInLiteral: Nit: Mark this case UNREACHABLE, i.e. case AccessMode::kStoreInLiteral: UNREACHABLE(); break;
https://codereview.chromium.org/2619773002/diff/120001/src/compiler/js-native... File src/compiler/js-native-context-specialization.cc (right): https://codereview.chromium.org/2619773002/diff/120001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:971: DCHECK_NE(access_mode, AccessMode::kStoreInLiteral); On 2017/01/11 05:06:12, Benedikt Meurer wrote: > Nit: The other way around, it's DCHECK_NE(unexpected, actual) Done. https://codereview.chromium.org/2619773002/diff/120001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:1357: DCHECK_NE(access_mode, AccessMode::kStoreInLiteral); On 2017/01/11 05:06:12, Benedikt Meurer wrote: > Nit: Same as above. Done. https://codereview.chromium.org/2619773002/diff/120001/src/compiler/js-native... src/compiler/js-native-context-specialization.cc:1455: case AccessMode::kStoreInLiteral: On 2017/01/11 05:06:12, Benedikt Meurer wrote: > Nit: Mark this case UNREACHABLE, i.e. > > case AccessMode::kStoreInLiteral: > UNREACHABLE(); > break; Done.
The CQ bit was checked by franzih@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.
The CQ bit was checked by franzih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2619773002/#ps140001 (title: "Fix DCHECKs.")
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": 140001, "attempt_start_ts": 1484126889820880,
"parent_rev": "9355f899efa82c6ed6f63943a15c6e98ce3cdea5", "commit_rev":
"088df4e13805b5a41bca3ed19030ff7c0c6f2df0"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Use feedback from StoreDataPropertyInLiteral. Lower StoreDataPropertyInLiteral() when storing computed property names in object literals. Add a new AccessMode, kStoreInLiteral. It is similar to AccessMode::kStore but does not look up properties on the prototype chain. 99% of all literal definitions with computed property names end up with generic access_info because of how we count properties. Once we fix https://bugs.chromium.org/p/v8/issues/detail?id=5625, they'll get lowered as well. BUG=v8:5624 ========== to ========== [turbofan] Use feedback from StoreDataPropertyInLiteral. Lower StoreDataPropertyInLiteral() when storing computed property names in object literals. Add a new AccessMode, kStoreInLiteral. It is similar to AccessMode::kStore but does not look up properties on the prototype chain. 99% of all literal definitions with computed property names end up with generic access_info because of how we count properties. Once we fix https://bugs.chromium.org/p/v8/issues/detail?id=5625, they'll get lowered as well. BUG=v8:5624 Review-Url: https://codereview.chromium.org/2619773002 Cr-Commit-Position: refs/heads/master@{#42210} Committed: https://chromium.googlesource.com/v8/v8/+/088df4e13805b5a41bca3ed19030ff7c0c6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/v8/v8/+/088df4e13805b5a41bca3ed19030ff7c0c6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
