Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533983006:
Optimize case conversion with icu_case_mapping (Closed)
|
Created:
4 years ago by jungshik at Google Modified:
4 years ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionOptimize case conversion with icu_case_mapping
Use FastAsciiConvert (as used by Unibrow) for i18n-aware
case conversion with --icu_case_mapping.
Move FastAsciiConvert to src/string-case.cc so that it can be used
by both runtime-{string,i18n}.
Add more tests.
BUG=v8:4477, v8:4476
TEST=intl/general/case*
Review-Url: https://codereview.chromium.org/2533983006
Cr-Commit-Position: refs/heads/master@{#41821}
Committed: https://chromium.googlesource.com/v8/v8/+/af38272dd94ed287fda9abd285d6071d96a7118f
Patch Set 1 #Patch Set 2 : index_to_first_upper fix #Patch Set 3 : handle short strings first for lowercase #Patch Set 4 : make runtim-i18n.cc identical to runtime-string.cc for lowercasing for ascii strings #Patch Set 5 : test for runtime-i18n vs runtime-string: do not check for new.target in i18n.js #Patch Set 6 : drop DEFINED(new.target) check in i18n.js; go back to ps3 #Patch Set 7 : do not use ASSIGN_RETURN_FAILURE_ON_EXCEPTION in ToUpper #
Total comments: 11
Patch Set 8 : move FastAsciiConvert to src/string-case* and do not check for exception #Patch Set 9 : a bit more refactoring #Patch Set 10 : fix a bug with index_to_first_unprocessed #Patch Set 11 : remove unnecessary and potentially expensive casting #Patch Set 12 : more tweaking ... #Patch Set 13 : a bit more refactoring #Patch Set 14 : fix v8.gyp #Patch Set 15 : a little tweak for Uppercase() with Latin-1; don't affect ASCII or Lower #Patch Set 16 : update comments with TODO #
Total comments: 6
Patch Set 17 : drop an unused variable: -Wunused-variable #
Messages
Total messages: 55 (34 generated)
The CQ bit was checked by jshin@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...
Earlier iterations were uploaded to another CL by mistake. https://codereview.chromium.org/2533033003/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jshin@chromium.org changed reviewers: + littledan@chromium.org, yangguo@chromium.org
The latest PS still needs a bit clean up/refactoring, but I have a couple of questions to Dan and Yang. Can you take a look?
https://codereview.chromium.org/2533983006/diff/120001/src/js/i18n.js File src/js/i18n.js (left): https://codereview.chromium.org/2533983006/diff/120001/src/js/i18n.js#oldcode... src/js/i18n.js:2126: } WIth this check in place, the JS portion of profiles has 5 ~ 6 extra functions (compared with non-i18n version) accounting for about 7% of the total run time. The corresponding function is strings.js does not have this check. Is it ok NOT to check for this? https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:1194: isolate->factory()->NewRawOneByteString(length).ToHandleChecked(); The first part of Runtime_STringToUpperCaseI18N for pure ASCII inpu is almost identical to the corresponding part in ConvertCase() in runtime-string.cc. The only difference is ASSIGN_RETURN_FAILURE_ON_EXCEPTION vs directly calling |isolate->factory()->NewRawOneByte....|. Somehow, that does makes a difference in performance. When I directly use NewRawOneByte.... (without checking for exception), the perf for pure ASCII input got on par with Unibrow. Otherwise, i18n code is about one sdt deviation slower than Unibrow. Is it ok NOT to use ASSIGN_RETURN_FAILURE_ON_EXCEPTION()? How can ConvertCase() in runtime-string.cc get away with it?
https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-ut... File src/runtime/runtime-utils.cc (right): https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-ut... src/runtime/runtime-utils.cc:32: template <bool is_lower> Initially, |is_lower| is a function argument, but I switched to a template param hoping that it may help improve performance (it does seem to improve perf) at the cost of increased code size.
https://codereview.chromium.org/2533983006/diff/120001/src/js/i18n.js File src/js/i18n.js (left): https://codereview.chromium.org/2533983006/diff/120001/src/js/i18n.js#oldcode... src/js/i18n.js:2126: } On 2016/12/02 06:52:38, jungshik at google wrote: > WIth this check in place, the JS portion of profiles has 5 ~ 6 extra functions > (compared with non-i18n version) accounting for about 7% of the total run time. > > The corresponding function is strings.js does not have this check. Is it ok NOT > to check for this? I don't understand why that check is in there. I can't find it in the current or any historical version of ECMA 402. I wonder if removing it would cause any test262 tests to fail, though--I can only imagine that this was included due to an older draft spec or possibly-incorrect test262 tests. Seems like a good idea to remove it. https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:1194: isolate->factory()->NewRawOneByteString(length).ToHandleChecked(); On 2016/12/02 06:52:38, jungshik at google wrote: > The first part of Runtime_STringToUpperCaseI18N for pure ASCII inpu is almost > identical to > the corresponding part in ConvertCase() in runtime-string.cc. > > The only difference is ASSIGN_RETURN_FAILURE_ON_EXCEPTION vs directly calling > |isolate->factory()->NewRawOneByte....|. Somehow, that does makes a difference > in performance. When I directly use NewRawOneByte.... (without checking for > exception), the perf for pure ASCII input got on par with Unibrow. Otherwise, > i18n code is about one sdt deviation slower than Unibrow. > > Is it ok NOT to use ASSIGN_RETURN_FAILURE_ON_EXCEPTION()? How can ConvertCase() > in runtime-string.cc get away with it? I think we would crash if out of memory, and the only way it would throw an exception is if the length is invalid. In this case, we know the length is valid because it is the length of another string. I see tons of instances of line 1194 in src/factory.cc, so I think it's OK here too, but I can understand authors who would want to use ASSIGN_RETURN_FAILURE_ON_EXCEPTION here because it's more obviously correct. Yang, any other thoughts? https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-ut... File src/runtime/runtime-utils.cc (right): https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-ut... src/runtime/runtime-utils.cc:32: template <bool is_lower> On 2016/12/02 06:54:41, jungshik at google wrote: > Initially, |is_lower| is a function argument, but I switched to a template param > hoping that it may help improve performance (it does seem to improve perf) at > the cost of increased code size. Sounds good to me! What kind of effect does it have on performance?
https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-ut... File src/runtime/runtime-utils.cc (right): https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-ut... src/runtime/runtime-utils.cc:1: // Copyright 2016 the V8 project authors. All rights reserved. Rather than moving this to src/runtime/runtime-utils.cc, what if you just leave it in runtime-strings.cc and made a runtime-strings.h to export the function, so it can be used in runtime-i18n.js? Or, to be even cleaner, it could be in a location like src/string-case.cc.
https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:1099: // shorter than a machine-word without any memory allocation overhead. What is the rationale for doing this only to short strings? Maybe elaborate a bit more? https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:1112: } So if the string is longer than a word, we always end up here with index_to_first_upper == length. This is somewhat misleading, when reading this code. https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:1125: // Instead of checking is_ascii here, we'd better modify FastAsciiConvert Is this a TODO? https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:1194: isolate->factory()->NewRawOneByteString(length).ToHandleChecked(); On 2016/12/02 23:35:08, Dan Ehrenberg wrote: > On 2016/12/02 06:52:38, jungshik at google wrote: > > The first part of Runtime_STringToUpperCaseI18N for pure ASCII inpu is almost > > identical to > > the corresponding part in ConvertCase() in runtime-string.cc. > > > > The only difference is ASSIGN_RETURN_FAILURE_ON_EXCEPTION vs directly calling > > |isolate->factory()->NewRawOneByte....|. Somehow, that does makes a > difference > > in performance. When I directly use NewRawOneByte.... (without checking for > > exception), the perf for pure ASCII input got on par with Unibrow. Otherwise, > > i18n code is about one sdt deviation slower than Unibrow. > > > > Is it ok NOT to use ASSIGN_RETURN_FAILURE_ON_EXCEPTION()? How can > ConvertCase() > > in runtime-string.cc get away with it? > > I think we would crash if out of memory, and the only way it would throw an > exception is if the length is invalid. In this case, we know the length is valid > because it is the length of another string. I see tons of instances of line 1194 > in src/factory.cc, so I think it's OK here too, but I can understand authors who > would want to use ASSIGN_RETURN_FAILURE_ON_EXCEPTION here because it's more > obviously correct. Yang, any other thoughts? Sounds right to me as well. No need to check for exceptions.
The CQ bit was checked by jshin@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 jshin@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 jshin@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 jshin@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.
Thank you, Yang and Dan, for the replies. I've done a lot of little tweaks. There are some improvements (especially Latin 1) but ASCII performance hasn't changed much. My benchmark results are at https://docs.google.com/spreadsheets/d/1KJCJxKc1FxFXjwmYqABS0_2cNdPetvnd8gY8_... "PS 13 (Dec 2016)" is the latest. The overall trend hasn't changed from my comment in the bug: * Pure ASCII - short lowercase string toLower: CL is faster - longer than 2 machine words: about the same - other cases: mine is 85 ~ 95% perf of the ToT * Latin 1: this CL is way faster. (130% ~ 400%; but my test examples are made up as opposed to taken from real world examples) * Beyond Latin1 (> U+00FF): ToT is way faster (the CL is 55~75% of ToT) but this CL is more accurate/correct. This is an ICU path. I will add a comment to the ICU bug on casing performance about the need to speed things up for BMP (<= U+FFFF) using some of v8's techniques. Could you take a look? Thanks a lot. On 2016/12/05 19:19:30, Yang wrote: > https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... > File src/runtime/runtime-i18n.cc (right): > > https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... > src/runtime/runtime-i18n.cc:1099: // shorter than a machine-word without any > memory allocation overhead. > What is the rationale for doing this only to short strings? Maybe elaborate a > bit more? The beginning of FastAsciiConvert() does something very similar but one word at a time. For pretty long strings (say, more than two words = 16 chars on 64-bit machines), directly using that is certainly faster despite a penalty from the memory allocation. For a string of ~ a machine-word length, it's kinda toss-up. (I gave up getting a meaningful stat from the measurement.). > https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... > src/runtime/runtime-i18n.cc:1112: } > So if the string is longer than a word, we always end up here with > index_to_first_upper == length. This is somewhat misleading, when reading this > code. > > https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... > src/runtime/runtime-i18n.cc:1125: // Instead of checking is_ascii here, we'd > better modify FastAsciiConvert > Is this a TODO? > > https://codereview.chromium.org/2533983006/diff/120001/src/runtime/runtime-i1... > src/runtime/runtime-i18n.cc:1194: > isolate->factory()->NewRawOneByteString(length).ToHandleChecked(); > On 2016/12/02 23:35:08, Dan Ehrenberg wrote: > > On 2016/12/02 06:52:38, jungshik at google wrote: > > > The first part of Runtime_STringToUpperCaseI18N for pure ASCII inpu is > almost > > > identical to > > > the corresponding part in ConvertCase() in runtime-string.cc. > > > > > > The only difference is ASSIGN_RETURN_FAILURE_ON_EXCEPTION vs directly > calling > > > |isolate->factory()->NewRawOneByte....|. Somehow, that does makes a > > difference > > > in performance. When I directly use NewRawOneByte.... (without checking for > > > exception), the perf for pure ASCII input got on par with Unibrow. > Otherwise, > > > i18n code is about one sdt deviation slower than Unibrow. > > > > > > Is it ok NOT to use ASSIGN_RETURN_FAILURE_ON_EXCEPTION()? How can > > ConvertCase() > > > in runtime-string.cc get away with it? > > > > I think we would crash if out of memory, and the only way it would throw an > > exception is if the length is invalid. In this case, we know the length is > valid > > because it is the length of another string. I see tons of instances of line > 1194 > > in src/factory.cc, so I think it's OK here too, but I can understand authors > who > > would want to use ASSIGN_RETURN_FAILURE_ON_EXCEPTION here because it's more > > obviously correct. Yang, any other thoughts? > > Sounds right to me as well. No need to check for exceptions. Thank you for the confirmation. I dropped the check.
lgtm
On 2016/12/14 at 01:39:29, Dan Ehrenberg wrote: > lgtm You probably wan to modify the commit message to mention src/string-case.cc rather than runtime-util.cc
Description was changed from
==========
Optimize case conversion with icu_case_mapping
Use FastAsciiConvert (as used by Unibrow) for i18n-aware
case conversion with --icu_case_mapping.
Move FastAsciiConvert to runtime-util.cc so that it can be used
by both runtime-{string,i18n}.
Add more tests.
BUG=v8:4477,v8:4476
TEST=intl/general/case*
==========
to
==========
Optimize case conversion with icu_case_mapping
Use FastAsciiConvert (as used by Unibrow) for i18n-aware
case conversion with --icu_case_mapping.
Move FastAsciiConvert to src/string-case.cc so that it can be used
by both runtime-{string,i18n}.
Add more tests.
BUG=v8:4477,v8:4476
TEST=intl/general/case*
==========
On 2016/12/14 01:39:50, Dan Ehrenberg wrote: > On 2016/12/14 at 01:39:29, Dan Ehrenberg wrote: > > lgtm > > You probably wan to modify the commit message to mention src/string-case.cc > rather than runtime-util.cc Thanks, Dan, for LGTM. I updated the description. PS 15 has a minimal tweak to *ToUpper (relative to PS 13). Sorry for the change (I'll stop making changes ). You may want to take a look at it. Yang, I'd appreciate if you can take a look, too.
https://codereview.chromium.org/2533983006/diff/300001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2533983006/diff/300001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:1047: int* sharp_s_count) { Using a pointer rather than a handle seems valid since it's within a DisallowHeapAllocation, but was this necessary for performance or something?
See my reply below. Thanks again for taking a look. https://codereview.chromium.org/2533983006/diff/300001/src/runtime/runtime-i1... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/2533983006/diff/300001/src/runtime/runtime-i1... src/runtime/runtime-i18n.cc:1047: int* sharp_s_count) { On 2016/12/15 19:24:25, Dan Ehrenberg wrote: > Using a pointer rather than a handle seems valid since it's within a > DisallowHeapAllocation, but was this necessary for performance or something? This change is triggered by a change below (call-site; see lines 1194 ~ 1198). Up to PS 14, on coming across non-ASCII characters, the result up to that point was discarded in ToUpper*. With the change in lines 1194 ~ 1198, I don't throw it away any more. That requires ToUpperOneByte to take a pointer to the middle of a string (the beginning of the unprocessed part of an input) instead of a while string. That also has a side-benefit of line 1066 being simpler.
On 2016/12/16 00:37:56, jungshik at google wrote: > See my reply below. Thanks again for taking a look. > > https://codereview.chromium.org/2533983006/diff/300001/src/runtime/runtime-i1... > File src/runtime/runtime-i18n.cc (right): > > https://codereview.chromium.org/2533983006/diff/300001/src/runtime/runtime-i1... > src/runtime/runtime-i18n.cc:1047: int* sharp_s_count) { > On 2016/12/15 19:24:25, Dan Ehrenberg wrote: > > Using a pointer rather than a handle seems valid since it's within a > > DisallowHeapAllocation, but was this necessary for performance or something? > > This change is triggered by a change below (call-site; see lines 1194 ~ 1198). > Up to PS 14, on coming across non-ASCII characters, the result up to that point > was discarded in ToUpper*. With the change in lines 1194 ~ 1198, I don't throw > it away any more. That requires ToUpperOneByte to take a pointer to the middle > of a string (the beginning of the unprocessed part of an input) instead of a > while string. That also has a side-benefit of line 1066 being simpler. lgtm.
https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js File src/js/i18n.js (left): https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js#oldcode... src/js/i18n.js:2125: throw %make_type_error(kOrdinaryFunctionCalledAsConstructor); Is this a spec change? So this function can be called as a constructor?
Thank you for taking a look. Below is my answer to the question. https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js File src/js/i18n.js (left): https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js#oldcode... src/js/i18n.js:2125: throw %make_type_error(kOrdinaryFunctionCalledAsConstructor); On 2016/12/16 14:02:21, Yang wrote: > Is this a spec change? So this function can be called as a constructor? I asked about this previously because having that check leads to a few additional entries in the profiling result (accounting for 6(?)% of the total ticks). Dan said that that check could be gone. StringTo{Lower,Upper}CaseJS() in src/js/string.js does not have that check. Because To{Lower,Upper}CaseI18N replace StringTo{Lower,Upper}CaseJS when --icu_case_mapping flag is on, I thought I could drop the check. // ECMA-262, 15.5.4.16 function StringToLowerCaseJS() { CHECK_OBJECT_COERCIBLE(this, "String.prototype.toLowerCase"); return %StringToLowerCase(TO_STRING(this)); }
The CQ bit was checked by jshin@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...
On 2016/12/16 18:12:36, jungshik at google wrote: > Thank you for taking a look. Below is my answer to the question. > > https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js > File src/js/i18n.js (left): > > https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js#oldcode... > src/js/i18n.js:2125: throw > %make_type_error(kOrdinaryFunctionCalledAsConstructor); > On 2016/12/16 14:02:21, Yang wrote: > > Is this a spec change? So this function can be called as a constructor? > > I asked about this previously because having that check leads to a few > additional entries in the profiling result (accounting for 6(?)% of the total > ticks). Dan said that that check could be gone. Below is what Dan said: <quote> I don't understand why that check is in there. I can't find it in the current or any historical version of ECMA 402. I wonder if removing it would cause any test262 tests to fail, though--I can only imagine that this was included due to an older draft spec or possibly-incorrect test262 tests. Seems like a good idea to remove it. </quote> > StringTo{Lower,Upper}CaseJS() in src/js/string.js does not have that check. > Because To{Lower,Upper}CaseI18N replace StringTo{Lower,Upper}CaseJS when > --icu_case_mapping flag is on, I thought I could drop the check. > > // ECMA-262, 15.5.4.16 > function StringToLowerCaseJS() { > CHECK_OBJECT_COERCIBLE(this, "String.prototype.toLowerCase"); > > return %StringToLowerCase(TO_STRING(this)); > }
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 jshin@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/2533983006/diff/300001/src/js/i18n.js File src/js/i18n.js (left): https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js#oldcode... src/js/i18n.js:2125: throw %make_type_error(kOrdinaryFunctionCalledAsConstructor); On 2016/12/16 18:12:36, jungshik at google wrote: > On 2016/12/16 14:02:21, Yang wrote: > > Is this a spec change? So this function can be called as a constructor? > > I asked about this previously because having that check leads to a few > additional entries in the profiling result (accounting for 6(?)% of the total > ticks). Dan said that that check could be gone. > > StringTo{Lower,Upper}CaseJS() in src/js/string.js does not have that check. > Because To{Lower,Upper}CaseI18N replace StringTo{Lower,Upper}CaseJS when > --icu_case_mapping flag is on, I thought I could drop the check. > > // ECMA-262, 15.5.4.16 > function StringToLowerCaseJS() { > CHECK_OBJECT_COERCIBLE(this, "String.prototype.toLowerCase"); > > return %StringToLowerCase(TO_STRING(this)); > } > > > > The %FunctionRemovePrototype call should be enough to prevent calls to this as a constructor. I'd like to remove all of them in a follow-on patch. It makes sense to do it earlier here because it will let us flip this flag on without the performance regression.
Thank you for confirming, Dan ! https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js File src/js/i18n.js (left): https://codereview.chromium.org/2533983006/diff/300001/src/js/i18n.js#oldcode... src/js/i18n.js:2125: throw %make_type_error(kOrdinaryFunctionCalledAsConstructor); On 2016/12/17 00:42:19, Dan Ehrenberg wrote: > On 2016/12/16 18:12:36, jungshik at google wrote: > > On 2016/12/16 14:02:21, Yang wrote: > > > Is this a spec change? So this function can be called as a constructor? > > > > I asked about this previously because having that check leads to a few > > additional entries in the profiling result (accounting for 6(?)% of the total > > ticks). Dan said that that check could be gone. > > > > StringTo{Lower,Upper}CaseJS() in src/js/string.js does not have that check. > > Because To{Lower,Upper}CaseI18N replace StringTo{Lower,Upper}CaseJS when > > --icu_case_mapping flag is on, I thought I could drop the check. > > > > // ECMA-262, 15.5.4.16 > > function StringToLowerCaseJS() { > > CHECK_OBJECT_COERCIBLE(this, "String.prototype.toLowerCase"); > > > > return %StringToLowerCase(TO_STRING(this)); > > } > > > > > > > > > > The %FunctionRemovePrototype call should be enough to prevent calls to this as a > constructor. I'd like to remove all of them in a follow-on patch. It makes sense > to do it earlier here because it will let us flip this flag on without the > performance regression. Thank you for confirming, Dan. I dropped the check in all the case-conversion related functions in i18n.js in this CL. i18n.js still has about 10 of them in other (localeCompare, Number.toLocaleString, Date.toLocaleString, etc) functions. I'll see if I can drop them in a separate CL.
The CQ bit was checked by jshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2533983006/#ps320001 (title: "drop an unused variable: -Wunused-variable")
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": 320001, "attempt_start_ts": 1482169730450150,
"parent_rev": "4c640be19bce9e64cec706793579011853846d9d", "commit_rev":
"af38272dd94ed287fda9abd285d6071d96a7118f"}
Message was sent while issue was closed.
Description was changed from
==========
Optimize case conversion with icu_case_mapping
Use FastAsciiConvert (as used by Unibrow) for i18n-aware
case conversion with --icu_case_mapping.
Move FastAsciiConvert to src/string-case.cc so that it can be used
by both runtime-{string,i18n}.
Add more tests.
BUG=v8:4477,v8:4476
TEST=intl/general/case*
==========
to
==========
Optimize case conversion with icu_case_mapping
Use FastAsciiConvert (as used by Unibrow) for i18n-aware
case conversion with --icu_case_mapping.
Move FastAsciiConvert to src/string-case.cc so that it can be used
by both runtime-{string,i18n}.
Add more tests.
BUG=v8:4477,v8:4476
TEST=intl/general/case*
Review-Url: https://codereview.chromium.org/2533983006
Cr-Commit-Position: refs/heads/master@{#41821}
Committed:
https://chromium.googlesource.com/v8/v8/+/af38272dd94ed287fda9abd285d6071d96a...
==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/v8/v8/+/af38272dd94ed287fda9abd285d6071d96a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
