Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1281343003:
Optimize RTL check in ICU to avoid mmap access. (Closed)
DescriptionOptimize RTL check in ICU to avoid mmap access.
Experiment suggests that we can improve Extensions.ExtensionServiceInitTime
by 15% @ the 75-th percentile by avoiding this mmap access.
Committed: https://crrev.com/399f02b7d4ba8d3ce8c2f7e4245c9bd992f5c8a0
Cr-Commit-Position: refs/heads/master@{#342949}
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #Patch Set 4 : rebase #Patch Set 5 : compile fixes #
Total comments: 2
Messages
Total messages: 25 (9 generated)
danduong@chromium.org changed reviewers: + jshin@chromium.org
Thank you. Some nits below. https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl.cc#newcode134 base/i18n/rtl.cc:134: static const char* kRTLLocales[] = { Please, add a note that this list has to be updated if we add more RTL locales. https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl.cc#newcode138 base/i18n/rtl.cc:138: "iw", 'iw' is a legacy name for 'he' and this API should never receive |locale_name| argument whose value is 'iw'. It's not a big deal and including 'iw' is defensive, but we'd rather fix the caller if this is ever called with 'iw'. https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl.h File base/i18n/rtl.h (right): https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl.h#newcode65 base/i18n/rtl.h:65: // Chrome-supported RTL locales. Please, make it even clearer that this is only for the start-up and GetTextDirectionForLocale() is the one to use otherwise. https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl_unittest.cc File base/i18n/rtl_unittest.cc (right): https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl_unittest.cc#n... base/i18n/rtl_unittest.cc:420: #endif I don't think we need these #if-0'd lines because we're not likely to localize to az_Arab and dv (Chromium may do, but not Google Chrome) in the near future. Get..Startup is for UI locales we localized to because it's used to determine the overall UI direction.
https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl.cc#newcode134 base/i18n/rtl.cc:134: static const char* kRTLLocales[] = { On 2015/08/11 at 19:23:13, jungshik at google wrote: > Please, add a note that this list has to be updated if we add more RTL locales. Done. https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl.cc#newcode138 base/i18n/rtl.cc:138: "iw", On 2015/08/11 at 19:23:13, jungshik at google wrote: > 'iw' is a legacy name for 'he' and this API should never receive |locale_name| argument whose value is 'iw'. It's not a big deal and including 'iw' is defensive, but we'd rather fix the caller if this is ever called with 'iw'. Acknowledged. I'd rather leave this here to be defensive. https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl_unittest.cc File base/i18n/rtl_unittest.cc (right): https://codereview.chromium.org/1281343003/diff/1/base/i18n/rtl_unittest.cc#n... base/i18n/rtl_unittest.cc:420: #endif On 2015/08/11 at 19:23:13, jungshik at google wrote: > I don't think we need these #if-0'd lines because we're not likely to localize to az_Arab and dv (Chromium may do, but not Google Chrome) in the near future. Get..Startup is for UI locales we localized to because it's used to determine the overall UI direction. Removed the if-0 lines.
https://codereview.chromium.org/1281343003/diff/20001/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/1281343003/diff/20001/base/i18n/rtl.cc#newcod... base/i18n/rtl.cc:143: if (StartsWith(locale_name, rtl_locale, CompareCase::SENSITIVE)) One more thing I forgot to mention. Sorry that I didn't mention it in the first round. Instead of StartsWith, you have to get the language code (chopping |locale_name| at the 1st '_' or '-') and compare the result against the list. That way, you can defend against a case where 3-letter language code (for some languages, there's no 2-letter code and we have to use 3-letter codes) happens to starts with one of the above.
https://codereview.chromium.org/1281343003/diff/20001/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/1281343003/diff/20001/base/i18n/rtl.cc#newcod... base/i18n/rtl.cc:143: if (StartsWith(locale_name, rtl_locale, CompareCase::SENSITIVE)) On 2015/08/11 20:38:29, jungshik at google wrote: > One more thing I forgot to mention. Sorry that I didn't mention it in the first > round. > > Instead of StartsWith, you have to get the language code (chopping |locale_name| > at the 1st '_' or '-') and compare the result against the list. That way, you > can defend against a case where 3-letter language code (for some languages, > there's no 2-letter code and we have to use 3-letter codes) happens to starts > with one of the above. > Because the only 3-letter language code in the current list of Chrome's UI languages is 'fil' and it's not a prefix match for any of the above. So, it's theoeretical. So, if you really think it can affect perf, you can comment this and use StartsWith, but I guess perf impact would be really tiny if you do the right thing.
https://codereview.chromium.org/1281343003/diff/20001/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/1281343003/diff/20001/base/i18n/rtl.cc#newcod... base/i18n/rtl.cc:143: if (StartsWith(locale_name, rtl_locale, CompareCase::SENSITIVE)) On 2015/08/11 at 20:41:59, jungshik at google wrote: > On 2015/08/11 20:38:29, jungshik at google wrote: > > One more thing I forgot to mention. Sorry that I didn't mention it in the first > > round. > > > > Instead of StartsWith, you have to get the language code (chopping |locale_name| > > at the 1st '_' or '-') and compare the result against the list. That way, you > > can defend against a case where 3-letter language code (for some languages, > > there's no 2-letter code and we have to use 3-letter codes) happens to starts > > with one of the above. > > > > Because the only 3-letter language code in the current list of Chrome's UI languages is 'fil' and it's not a prefix match for any of the above. So, it's theoeretical. So, if you really think it can affect perf, you can comment this and use StartsWith, but I guess perf impact would be really tiny if you do the right thing. That was definitely an oversight on my part. Thanks for the catch!
Thanks. LGTM
The CQ bit was checked by danduong@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281343003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281343003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by danduong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org Link to the patchset: https://codereview.chromium.org/1281343003/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281343003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281343003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by danduong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jshin@chromium.org Link to the patchset: https://codereview.chromium.org/1281343003/#ps80001 (title: "compile fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1281343003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1281343003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/399f02b7d4ba8d3ce8c2f7e4245c9bd992f5c8a0 Cr-Commit-Position: refs/heads/master@{#342949}
Message was sent while issue was closed.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1281343003/diff/80001/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/1281343003/diff/80001/base/i18n/rtl.cc#newcod... base/i18n/rtl.cc:157: TextDirection GetTextDirectionForLocale(const char* locale_name) { Seems like the only place where GetTextDirectionForLocale() gets used after this change (besides unit tests), is message_bundle.cc. Does it still make sense to have this separate logic or can that call site also be switched to the GetTextDirectionForLocaleInStartUp() codepath?
Message was sent while issue was closed.
https://codereview.chromium.org/1281343003/diff/80001/base/i18n/rtl.cc File base/i18n/rtl.cc (right): https://codereview.chromium.org/1281343003/diff/80001/base/i18n/rtl.cc#newcod... base/i18n/rtl.cc:157: TextDirection GetTextDirectionForLocale(const char* locale_name) { On 2015/08/12 at 15:40:02, Alexei Svitkine wrote: > Seems like the only place where GetTextDirectionForLocale() gets used after this change (besides unit tests), is message_bundle.cc. > > Does it still make sense to have this separate logic or can that call site also be switched to the GetTextDirectionForLocaleInStartUp() codepath? That's a good question. jshin@ I know your original suggestion was to have 2 different methods for this, but is it possible that we just make GetTextDirectionForLocaleInStartUp the only method? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
