Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2958063002:
Fast path for Map.prototype.(Get|Has) with Smi key. (Closed)
|
Created:
3 years, 5 months ago by Jarin Modified:
3 years, 5 months ago Reviewers:
Camillo Bruni CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionFast path for Map.prototype.(Get|Has) with Smi key.
Review-Url: https://codereview.chromium.org/2958063002
Cr-Commit-Position: refs/heads/master@{#46287}
Committed: https://chromium.googlesource.com/v8/v8/+/4e9e81454049b59b256a71a604a9c36a341f923e
Patch Set 1 #Patch Set 2 : Tweak #Patch Set 3 : Tweaks #
Total comments: 2
Messages
Total messages: 20 (15 generated)
The CQ bit was checked by jarin@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...
jarin@chromium.org changed reviewers: + cbruni@chromium.org
Could you take a look, please?
Description was changed from ========== Fast path for Map.prototype.(Get|Has) - written in CSA. ========== to ========== Fast path for Map.prototype.(Get|Has) with Smi key. ==========
The CQ bit was checked by jarin@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 jarin@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.
LGMT + test nit https://codereview.chromium.org/2958063002/diff/40001/src/builtins/builtins-c... File src/builtins/builtins-collections-gen.cc (right): https://codereview.chromium.org/2958063002/diff/40001/src/builtins/builtins-c... src/builtins/builtins-collections-gen.cc:388: ChangeInt32ToIntPtr(ComputeIntegerHash(key, Int32Constant(0))); uh, I think we should add a cctest to ensure the C++ ComputeIntegerHash does the same. I'll whip up a CL. https://codereview.chromium.org/2958063002/diff/40001/src/builtins/builtins-c... src/builtins/builtins-collections-gen.cc:515: } Now that we have the Smi fast-path there is no way to reliably test the C++ code anymore. We should probably start adding dedicated cctests that compare CSA vs C++ lookups. see: test/cctest/test-code-stub-assembler.cc
lgtm
The CQ bit was checked by jarin@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": 40001, "attempt_start_ts": 1498657766625000,
"parent_rev": "9e5e3aff96b9613bb97dcfeebe8f280cbb7caf7c", "commit_rev":
"4e9e81454049b59b256a71a604a9c36a341f923e"}
Message was sent while issue was closed.
Description was changed from ========== Fast path for Map.prototype.(Get|Has) with Smi key. ========== to ========== Fast path for Map.prototype.(Get|Has) with Smi key. Review-Url: https://codereview.chromium.org/2958063002 Cr-Commit-Position: refs/heads/master@{#46287} Committed: https://chromium.googlesource.com/v8/v8/+/4e9e81454049b59b256a71a604a9c36a341... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/4e9e81454049b59b256a71a604a9c36a341... |
