Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652213003:
[stubs] KeyedLoadIC_Generic: prototype chain lookup support (Closed)
|
Created:
3 years, 11 months ago by Jakob Kummerow Modified:
3 years, 10 months ago Reviewers:
Igor Sheludko CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[stubs] KeyedLoadIC_Generic: prototype chain lookup support (reland)
Performing lookups on the prototype chain in the stub avoids a
bunch of slow-path runtime calls. For now, only receivers with
dictionary-mode properties do this; fast-mode receivers will follow
if it's beneficial.
(previously landed as r42751 / 82e10f5fbac)
Review-Url: https://codereview.chromium.org/2652213003
Cr-Commit-Position: refs/heads/master@{#42785}
Committed: https://chromium.googlesource.com/v8/v8/+/e42da75c9eaa604353623b0bb17051513f05410a
Patch Set 1 #
Total comments: 5
Patch Set 2 : address nit #Patch Set 3 : rebased for reland #Patch Set 4 : fix private symbols #Messages
Total messages: 23 (11 generated)
jkummerow@chromium.org changed reviewers: + ishell@chromium.org
PTAL. Modeled after CSA::TryPrototypeChainLookup, but sufficiently different that I'm not sure unification is worth it. https://codereview.chromium.org/2652213003/diff/1/src/ic/accessor-assembler.cc File src/ic/accessor-assembler.cc (right): https://codereview.chromium.org/2652213003/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1578: Bind(&next_proto); This trampoline and the next are needed to appease Turbofan's variable resolver.
lgtm with nits: https://codereview.chromium.org/2652213003/diff/1/src/ic/accessor-assembler.cc File src/ic/accessor-assembler.cc (right): https://codereview.chromium.org/2652213003/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1564: GotoIf(Word32Equal(var_holder_instance_type.value(), I guess we can move this check to next_proto block. https://codereview.chromium.org/2652213003/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1578: Bind(&next_proto); On 2017/01/25 16:40:06, Jakob Kummerow wrote: > This trampoline and the next are needed to appease Turbofan's variable resolver. Maybe add this as a comment to the code.
Thanks, landing. https://codereview.chromium.org/2652213003/diff/1/src/ic/accessor-assembler.cc File src/ic/accessor-assembler.cc (right): https://codereview.chromium.org/2652213003/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1564: GotoIf(Word32Equal(var_holder_instance_type.value(), On 2017/01/27 08:58:08, Igor Sheludko wrote: > I guess we can move this check to next_proto block. No, we need it in the first iteration. "Integer indexed exotic" is weird. https://codereview.chromium.org/2652213003/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1578: Bind(&next_proto); On 2017/01/27 08:58:08, Igor Sheludko wrote: > On 2017/01/25 16:40:06, Jakob Kummerow wrote: > > This trampoline and the next are needed to appease Turbofan's variable > resolver. > > Maybe add this as a comment to the code. Done.
The CQ bit was checked by jkummerow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2652213003/#ps20001 (title: "address nit")
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
Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/20141) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
The CQ bit was checked by jkummerow@chromium.org
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": 1485571847400450,
"parent_rev": "01a93925c1f502516f4d29dfa79c7271ed6b0e5f", "commit_rev":
"82e10f5fbacbbd71bc65e99322f432470692bf41"}
Message was sent while issue was closed.
Description was changed from ========== [stubs] KeyedLoadIC_Generic: prototype chain lookup support Performing lookups on the prototype chain in the stub avoids a bunch of slow-path runtime calls. For now, only receivers with dictionary-mode properties do this; fast-mode receivers will follow if it's beneficial. ========== to ========== [stubs] KeyedLoadIC_Generic: prototype chain lookup support Performing lookups on the prototype chain in the stub avoids a bunch of slow-path runtime calls. For now, only receivers with dictionary-mode properties do this; fast-mode receivers will follow if it's beneficial. Review-Url: https://codereview.chromium.org/2652213003 Cr-Commit-Position: refs/heads/master@{#42751} Committed: https://chromium.googlesource.com/v8/v8/+/82e10f5fbacbbd71bc65e99322f43247069... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/82e10f5fbacbbd71bc65e99322f43247069...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2657393002/ by machenbach@chromium.org. The reason for reverting is: Speculative revert for breaking a layout test: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui... Unfortunately, the test results archive is not giving much info this time..
Message was sent while issue was closed.
Description was changed from ========== [stubs] KeyedLoadIC_Generic: prototype chain lookup support Performing lookups on the prototype chain in the stub avoids a bunch of slow-path runtime calls. For now, only receivers with dictionary-mode properties do this; fast-mode receivers will follow if it's beneficial. Review-Url: https://codereview.chromium.org/2652213003 Cr-Commit-Position: refs/heads/master@{#42751} Committed: https://chromium.googlesource.com/v8/v8/+/82e10f5fbacbbd71bc65e99322f43247069... ========== to ========== [stubs] KeyedLoadIC_Generic: prototype chain lookup support (reland) Performing lookups on the prototype chain in the stub avoids a bunch of slow-path runtime calls. For now, only receivers with dictionary-mode properties do this; fast-mode receivers will follow if it's beneficial. (previously landed as r42751 / 82e10f5fbac) ==========
Ah, yes, of course, private symbols require special handling. PTAL at patch set 3->4.
lgtm
The CQ bit was checked by jkummerow@chromium.org
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": 60001, "attempt_start_ts": 1485800792554010,
"parent_rev": "dc85f4c8338c1c824af4f7ee3274dc9f95d14e49", "commit_rev":
"e42da75c9eaa604353623b0bb17051513f05410a"}
Message was sent while issue was closed.
Description was changed from ========== [stubs] KeyedLoadIC_Generic: prototype chain lookup support (reland) Performing lookups on the prototype chain in the stub avoids a bunch of slow-path runtime calls. For now, only receivers with dictionary-mode properties do this; fast-mode receivers will follow if it's beneficial. (previously landed as r42751 / 82e10f5fbac) ========== to ========== [stubs] KeyedLoadIC_Generic: prototype chain lookup support (reland) Performing lookups on the prototype chain in the stub avoids a bunch of slow-path runtime calls. For now, only receivers with dictionary-mode properties do this; fast-mode receivers will follow if it's beneficial. (previously landed as r42751 / 82e10f5fbac) Review-Url: https://codereview.chromium.org/2652213003 Cr-Commit-Position: refs/heads/master@{#42785} Committed: https://chromium.googlesource.com/v8/v8/+/e42da75c9eaa604353623b0bb17051513f0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/e42da75c9eaa604353623b0bb17051513f0... |
