Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1971943002:
Make normalize, collator:compare and breakiterator a bit more efficient (Closed)
|
Created:
4 years, 7 months ago by jungshik at Google Modified:
4 years, 7 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionMake normalize, collator, breakiterator faster
1. Normalizer: Use ICU's normalizer2 API instead of deprecated
normalizer. Also uses quick scan method in the API to speed up in the
most common cases (almost normalized and the target is NFC)
2. In all three cases, replace |v8::Utils::ToLocal(..)| with a more
efficient internal method.
BUG=v8:4983
TEST=intl/string/normal*, intl/collator/*, intl/break-iterator/*
TEST=test262/intl402/Collator/*, test262/built-ins/String/prototype/normalize/*
CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng
Committed: https://crrev.com/a4e0ee12e4489a74069906fd12661b73fe51500f
Cr-Commit-Position: refs/heads/master@{#36298}
Patch Set 1 #Patch Set 2 : Collator and Normalizer: use more efficient way to extract UChar16 buffer #Patch Set 3 : ignore this PS ! #Patch Set 4 : go back to PS #2; rebased #
Total comments: 6
Patch Set 5 : use RUNTIME_ASSERT #Messages
Total messages: 23 (9 generated)
Description was changed from ========== Use icu's new Normalizer2 API instead of old API Also, replace |v8::Utils::ToLocal(..)| with a more efficient internal method. BUG=4983 TEST=intl/string/normal* ========== to ========== Make normalize, collator, breakiterator faster 1. Normalizer: Use ICU's normalizer2 API instead of deprecated normalizer. Also uses quick scan method in the API to speed up in the most common cases (almost normalized and the target is NFC) 2. In all three cases, replace |v8::Utils::ToLocal(..)| with a more efficient internal method. BUG=4983 TEST=intl/string/normal* ==========
jshin@chromium.org changed reviewers: + littledan@chromium.org, yangguo@chromium.org
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/patch-status/1971943002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971943002/20001
PTAL, Thank you
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make normalize, collator, breakiterator faster 1. Normalizer: Use ICU's normalizer2 API instead of deprecated normalizer. Also uses quick scan method in the API to speed up in the most common cases (almost normalized and the target is NFC) 2. In all three cases, replace |v8::Utils::ToLocal(..)| with a more efficient internal method. BUG=4983 TEST=intl/string/normal* ========== to ========== Make normalize, collator, breakiterator faster 1. Normalizer: Use ICU's normalizer2 API instead of deprecated normalizer. Also uses quick scan method in the API to speed up in the most common cases (almost normalized and the target is NFC) 2. In all three cases, replace |v8::Utils::ToLocal(..)| with a more efficient internal method. BUG=v8:4983 TEST=intl/string/normal* ==========
On 2016/05/12 01:07:04, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. I'm confused... is this ready for review?
On 2016/05/13 06:33:38, Yang wrote: > On 2016/05/12 01:07:04, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > I'm confused... is this ready for review? Sorry about PS 3 (I meant to make a new branch for that. PS 3 contains totally unrelated changes). To make it clear, I'll upload PS #4 that is identical to PS 2.
On 2016/05/13 23:41:18, jshin (jungshik at google) wrote: > On 2016/05/13 06:33:38, Yang wrote: > > On 2016/05/12 01:07:04, commit-bot: I haz the power wrote: > > > Dry run: This issue passed the CQ dry run. > > > > I'm confused... is this ready for review? > > Sorry about PS 3 (I meant to make a new branch for that. PS 3 contains totally > unrelated changes). To make it clear, I'll upload PS #4 that is identical to PS > 2. PS 4 is ready for review. PTAL. Thank you !
Seems good to me, but I'd prefer if you wait until a review from Yang as well. Have you taken a good look at the intl tests to evaluate whether they cover this surface well? https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... src/runtime/runtime-i18n.cc:606: {"nfc", UNORM2_DECOMPOSE}, nfd? https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... src/runtime/runtime-i18n.cc:608: {"nfkc", UNORM2_DECOMPOSE}, nfkd?
Thank you, Dan, for taking a look. 'normalize' and 'compare' are well covered by test262/intl402. intl/break-iterator has a decent coverage. https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... src/runtime/runtime-i18n.cc:606: {"nfc", UNORM2_DECOMPOSE}, On 2016/05/16 22:55:38, Dan Ehrenberg wrote: > nfd? The first element ('name') indicates the type of normalization (canonical, compat. There are more types supported by ICU but Ecma 402 does not yet cover others). The second ('mode') indicates 'mode' (or direction; whether it's compose or decompose). Yeah, it is confusing. My 1st CL (not uploaded?) did use 'nfd' by mistake and |Normalizer2:getInstance| failed because 'nfd' is not https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... src/runtime/runtime-i18n.cc:608: {"nfkc", UNORM2_DECOMPOSE}, On 2016/05/16 22:55:38, Dan Ehrenberg wrote: > nfkd? ditto :-)
LGTM with comment. https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... src/runtime/runtime-i18n.cc:633: if (!normalizer) return isolate->ThrowIllegalOperation(); Do you expect this to happen? Can we have a RUNTIME_ASSERT here instead?
Description was changed from ========== Make normalize, collator, breakiterator faster 1. Normalizer: Use ICU's normalizer2 API instead of deprecated normalizer. Also uses quick scan method in the API to speed up in the most common cases (almost normalized and the target is NFC) 2. In all three cases, replace |v8::Utils::ToLocal(..)| with a more efficient internal method. BUG=v8:4983 TEST=intl/string/normal* ========== to ========== Make normalize, collator, breakiterator faster 1. Normalizer: Use ICU's normalizer2 API instead of deprecated normalizer. Also uses quick scan method in the API to speed up in the most common cases (almost normalized and the target is NFC) 2. In all three cases, replace |v8::Utils::ToLocal(..)| with a more efficient internal method. BUG=v8:4983 TEST=intl/string/normal*, intl/collator/*, intl/break-iterator/* TEST=test262/intl402/Collator/*, test262/built-ins/String/prototype/normalize/* CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng ==========
On 2016/05/17 05:09:33, Yang wrote: > LGTM with comment. > > https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... > File src/runtime/runtime-i18n.cc (right): > > https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... > src/runtime/runtime-i18n.cc:633: if (!normalizer) return > isolate->ThrowIllegalOperation(); > Do you expect this to happen? Can we have a RUNTIME_ASSERT here instead? Thank you, Yang !
Yang's comment was addressed, btw. https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... File src/runtime/runtime-i18n.cc (right): https://codereview.chromium.org/1971943002/diff/60001/src/runtime/runtime-i18... src/runtime/runtime-i18n.cc:633: if (!normalizer) return isolate->ThrowIllegalOperation(); On 2016/05/17 05:09:33, Yang wrote: > Do you expect this to happen? Can we have a RUNTIME_ASSERT here instead? That should not happen unless somehow ICU data is not accessible. So, I'll turn that into RUNTIME_ASSERT. Thank you for the suggestion.
The CQ bit was checked by jshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/1971943002/#ps80001 (title: "use RUNTIME_ASSERT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971943002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971943002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make normalize, collator, breakiterator faster 1. Normalizer: Use ICU's normalizer2 API instead of deprecated normalizer. Also uses quick scan method in the API to speed up in the most common cases (almost normalized and the target is NFC) 2. In all three cases, replace |v8::Utils::ToLocal(..)| with a more efficient internal method. BUG=v8:4983 TEST=intl/string/normal*, intl/collator/*, intl/break-iterator/* TEST=test262/intl402/Collator/*, test262/built-ins/String/prototype/normalize/* CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng ========== to ========== Make normalize, collator, breakiterator faster 1. Normalizer: Use ICU's normalizer2 API instead of deprecated normalizer. Also uses quick scan method in the API to speed up in the most common cases (almost normalized and the target is NFC) 2. In all three cases, replace |v8::Utils::ToLocal(..)| with a more efficient internal method. BUG=v8:4983 TEST=intl/string/normal*, intl/collator/*, intl/break-iterator/* TEST=test262/intl402/Collator/*, test262/built-ins/String/prototype/normalize/* CQ_INCLUDE_TRYBOTS=tryserver.v8:v8_linux_noi18n_rel_ng Committed: https://crrev.com/a4e0ee12e4489a74069906fd12661b73fe51500f Cr-Commit-Position: refs/heads/master@{#36298} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a4e0ee12e4489a74069906fd12661b73fe51500f Cr-Commit-Position: refs/heads/master@{#36298} |
