Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2705193002:
[string] Unpack external strings in String.prototype.indexOf (Closed)
|
Created:
3 years, 10 months ago by jgruber Modified:
3 years, 10 months ago Reviewers:
Camillo Bruni CC:
Igor Sheludko, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[string] Unpack external strings in String.prototype.indexOf
This CL allows the String.p.indexOf fast path to handle one-byte
external strings (in addition to one-byte sequential strings).
BUG=
Review-Url: https://codereview.chromium.org/2705193002
Cr-Commit-Position: refs/heads/master@{#43369}
Committed: https://chromium.googlesource.com/v8/v8/+/acd859895b23171b05626f911a1bfba89da68f24
Patch Set 1 #Patch Set 2 : Restructure TryDerefExternalString #Patch Set 3 : Switch-based handling for external strings #Patch Set 4 : Unused variable #Patch Set 5 : Fix assert #
Total comments: 8
Patch Set 6 : Fix wrong variable #Patch Set 7 : Add private helpers for string instance types #
Total comments: 2
Messages
Total messages: 39 (32 generated)
The CQ bit was checked by jgruber@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_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by jgruber@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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by jgruber@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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by jgruber@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_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
The CQ bit was checked by jgruber@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.
jgruber@chromium.org changed reviewers: + cbruni@chromium.org
The CQ bit was checked by jgruber@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/2705193002/diff/80001/src/builtins/builtins-s... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2705193002/diff/80001/src/builtins/builtins-s... src/builtins/builtins-string.cc:918: var_string_addr.Bind(OneByteCharAddress(unpacked, start_position)); OneByteCharAddress https://codereview.chromium.org/2705193002/diff/80001/src/builtins/builtins-s... src/builtins/builtins-string.cc:945: ChangeInt32ToIntPtr(LoadOneByteChar(unpacked, int_zero))); OneByteCharAddress https://codereview.chromium.org/2705193002/diff/80001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2705193002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:3519: Int32Constant(kExternalStringTag))); nit: would be nice to have a IsExnteralStringType helper or so? https://codereview.chromium.org/2705193002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:3524: Int32Constant(0)), nit: same here...
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 jgruber@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...
Addressed comments, ptal. https://codereview.chromium.org/2705193002/diff/80001/src/builtins/builtins-s... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2705193002/diff/80001/src/builtins/builtins-s... src/builtins/builtins-string.cc:918: var_string_addr.Bind(OneByteCharAddress(unpacked, start_position)); On 2017/02/22 10:16:20, Camillo Bruni wrote: > OneByteCharAddress Stray comment? https://codereview.chromium.org/2705193002/diff/80001/src/builtins/builtins-s... src/builtins/builtins-string.cc:945: ChangeInt32ToIntPtr(LoadOneByteChar(unpacked, int_zero))); On 2017/02/22 10:16:20, Camillo Bruni wrote: > OneByteCharAddress Changed to var_needle_byte as discussed offline, great catch. There's no tests for this since this case is currently unreachable - we only take the fast path for 1-char needles, and short external strings bailout to runtime. https://codereview.chromium.org/2705193002/diff/80001/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2705193002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:3519: Int32Constant(kExternalStringTag))); On 2017/02/22 10:16:20, Camillo Bruni wrote: > nit: would be nice to have a IsExnteralStringType helper or so? Done. Added in private namespace to not pollute CSA. https://codereview.chromium.org/2705193002/diff/80001/src/code-stub-assembler... src/code-stub-assembler.cc:3524: Int32Constant(0)), On 2017/02/22 10:16:20, Camillo Bruni wrote: > nit: same here... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2705193002/diff/120001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2705193002/diff/120001/src/code-stub-assemble... src/code-stub-assembler.cc:3518: } we can probably push those even on the CodeStubAssembler for reuse :)
https://codereview.chromium.org/2705193002/diff/120001/src/code-stub-assemble... File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2705193002/diff/120001/src/code-stub-assemble... src/code-stub-assembler.cc:3518: } On 2017/02/22 12:02:05, Camillo Bruni wrote: > we can probably push those even on the CodeStubAssembler for reuse :) Agreed, but atm it doesn't look like we'd save much. I'd like to refactor these string extraction things into one method that we can - hopefully - use everywhere.
The CQ bit was checked by jgruber@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": 120001, "attempt_start_ts": 1487765177464720,
"parent_rev": "c8329253ea345e06a923f7800f96f4ef59262997", "commit_rev":
"acd859895b23171b05626f911a1bfba89da68f24"}
Message was sent while issue was closed.
Description was changed from ========== [string] Unpack external strings in String.prototype.indexOf This CL allows the String.p.indexOf fast path to handle one-byte external strings (in addition to one-byte sequential strings). BUG= ========== to ========== [string] Unpack external strings in String.prototype.indexOf This CL allows the String.p.indexOf fast path to handle one-byte external strings (in addition to one-byte sequential strings). BUG= Review-Url: https://codereview.chromium.org/2705193002 Cr-Commit-Position: refs/heads/master@{#43369} Committed: https://chromium.googlesource.com/v8/v8/+/acd859895b23171b05626f911a1bfba89da... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/acd859895b23171b05626f911a1bfba89da... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
