Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1541393003:
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree (Closed)
|
Created:
4 years, 12 months ago by kojii Modified:
4 years, 10 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, danakj, Rik, f(malita), blink-reviews, vmpstr+blinkwatch_chromium.org, kinuko+watch, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster.
The previous CL[1] made it faster by ~90% for codepoints below U+2020,
but codepoints abvoe U+2020 were not as fast. This CL makes all
codepoints faster, as fast as ICU functions.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). This patch changes to use
UTrie2, which is the data structure ICU property functions use.
In addition in this patch:
* U+2763 and U+2764 are added as requested by drott@.
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
[1] https://codereview.chromium.org/1545073002
BUG=571943
Committed: https://crrev.com/e6dc3f425137021f39eddbbeb2035273ce36f986
Cr-Commit-Position: refs/heads/master@{#371917}
Patch Set 1 #Patch Set 2 : UTrie2 for isCJKIdeographOrSymbol #Patch Set 3 : isCJKIdeograph and isUprightInMixedVertical #Patch Set 4 : Fix minor errors #Patch Set 5 : Fix Linux/Mac compile error #Patch Set 6 : Cleanup, remove isCJKIdeograph, remove perf test code #
Total comments: 12
Patch Set 7 : drott review #Patch Set 8 : Construct the UTrie2 from the serialized byte array #Patch Set 9 : Android compiler doesn't like trailing backslash in comments #Patch Set 10 : Rebase #Patch Set 11 : Rebase #Patch Set 12 : gn support, gyp NYI #Patch Set 13 : gyp #Patch Set 14 : cross-compile try #Patch Set 15 : gn cross-compiler trial cont'd #Patch Set 16 : gyp cross-compile still fails, another try #Patch Set 17 : gyp+cross-compile, another try #Patch Set 18 : gyp cross-compile still fails, another try #Patch Set 19 : Add #host to icuuc #Patch Set 20 : Revert to PS11 #Patch Set 21 : Revert to PS19, add support for use_system_icu #Patch Set 22 : Use pre-generated file if (gyp and (android or chromeos)) #Patch Set 23 : Changed the criteria to target_arch!=host_arch #Patch Set 24 : Tweak conditions #Messages
Total messages: 53 (18 generated)
Description was changed from ========== (NOT FOR LANDING) Performance measure for isCJKIdeograph BUG=571943 ========== to ========== Improve performance of Character::isCJKIdeographOrSymbol using trie This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the method is now almost equivalent as the ICU functions. In addition: * Character::isUprightInMixedVertical() uses the UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ==========
Description was changed from ========== Improve performance of Character::isCJKIdeographOrSymbol using trie This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the method is now almost equivalent as the ICU functions. In addition: * Character::isUprightInMixedVertical() uses the UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ========== to ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the method is now almost equivalent as the ICU functions. In addition: * Character::isUprightInMixedVertical() uses the UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ==========
Description was changed from ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the method is now almost equivalent as the ICU functions. In addition: * Character::isUprightInMixedVertical() uses the UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ========== to ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the method is now almost equivalent as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() uses the UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ==========
Description was changed from ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the method is now almost equivalent as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() uses the UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ========== to ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the function is now as fast as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() uses the UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ==========
Description was changed from ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the function is now as fast as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() uses the UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ========== to ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the function is now as fast as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() was switched to UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ==========
Description was changed from ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the function is now as fast as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() was switched to UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch as I figure how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ========== to ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the function is now as fast as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() was switched to UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch when I figure out how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ==========
Description was changed from ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. All 2569 => 292 (88% improve) ASCII 68 => 68 (0%) Han 2958 => 263 (91% improve) Hira 258 => 11 (95% improve) Arabic 37 => 32 (13% improve) The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the function is now as fast as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() was switched to UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch when I figure out how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ========== to ========== Improve performance of Character::isCJKIdeographOrSymbol by using trie tree This patch is another effort to make Character::isCJKIdeographOrSymbol faster by using UTrie2, a trie tree in ICU. Before After Improve ICU All 2569 => 292 88% 298 ASCII 68 => 68 0% 160 Han 2958 => 263 91% 344 Hira 258 => 11 95% 14 Arabic 37 => 32 13% 44 The previous CL[1] clarified that binary search is not as fast as ICU functions such as uscript_getScript(). Since UTrie2 is the data structure ICU property functions use, the performance of the function is now as fast as the ICU functions. In addition in this patch: * Character::isUprightInMixedVertical() was switched to UTrie2 too. * Character::isCJKIdeograph() was removed because it is no longer used. Ideally we should load the UTrie2 from a serialized frozen byte array which can be built during the build time. It should be in a future patch when I figure out how to do it, but the benefit to switch is obvious without that work. [1] https://codereview.chromium.org/1545073002 BUG=571943 ==========
Description was changed from
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
to
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* Iterations vary, comparing rows is meaningless.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
Description was changed from
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* Iterations vary, comparing rows is meaningless.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
to
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary, comparing rows is meaningless.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
Description was changed from
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary, comparing rows is meaningless.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
to
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
Description was changed from
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
to
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster, this time by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
kojii@chromium.org changed reviewers: + drott@chromium.org, eae@chromium.org
PTAL.
I think it's a great direction for improvement of this code. Thanks, Koji! Maybe I missed some earlier discussion but could you explain the values/unit of the performance figures you're presenting? What subset of codepoints is used? I believe there are good improvements, but the percentage values are a bit hard to interpret for me without the context of what is being measured. See below for my concern regarding the ad-hoc large temporary allocation. It'd be great if we can avoid set, either by fusing our range information here differently, or perhaps reconsidering UnicodeSet to avoid writing too much code for this purpose. Did you compare the performance of UnicodeSet to UTrie2? https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/Character.cpp (right): https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:38: #define MUTEX_H Why is this needed? https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:98: 0x2672, 0x267D, Could you perhaps include // Emoji HEAVY HEART EXCLAMATION MARK ORNAMENT..HEAVY BLACK HEART // Needed in order not to break Emoji heart-kiss sequences in // CachingWordShapeIterator. // cmp. http://www.unicode.org/emoji/charts/emoji-zwj-sequences.html 0x2763, 0x2764, here already in order to avoid conflicts with my emoji ZWJ sequences patch? https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:100: // Ideographic Description Characters, with CJK Symbols and Punctuation, excluding 0x3030. I know that this is previous code in a way. However, as we're touching it here and Blink is converging with Chromium's style. Could you ensure an 80 column limit in the whole file? https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:125: // UNICODE VERTICAL TEXT LAYOUT http://www.unicode.org/reports/tr50/ I wouldn't uppercase the comment here. Perhaps: // Individual codepoints < 0xFF from // needed for Unicode vertical text layout according to // http://www.unicode.org/reports/tr50/ // Taken from the corresponding data file: // http://www.unicode.org/Public/vertical/revision-15/VerticalOrientation-15.txt https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:268: const UChar32 maxCodePointForPropertyValues = 0x10FFFD; As far as I can see this is only used for the assertions and the dependency to the 0x10FFFD value in isUprightInMixedVerticalRanges is not documented or clear here. I would find it clearer to not redefine a value here and use kMaxCodepoint from CharacterNames and use that for the ASSERTions. https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:270: static void setRanges(CharacterProperties* values, const UChar32* ranges, size_t length, CharacterProperties value) If I understand correctly, the CharacterProperties array is a rather heavy 8,5MB temporary data structure to fuse the two characteristics isCJKIdeographOrSymbol and isUprightInMixedVertical. I'm a little wary of this large ad-hoc allocation that can be avoided. It could be faster (because gaps could be skipped) and leaner in terms of memory if we had two iterators that could find the next nearest boundary for isCJKIdeographOrSymbol and isUprightInMixedVertical, keep track of the mode of the property values (i.e. 0x01, 0x02 or 0x03) and call utrie2_setRange32 accordingly. To simplify, isolated codepoints could be defined as trivial one-codepoint-ranges, too. If that's too much code to write, UnicodeSet probably does this work for us if we define the ranges in the pattern format that UnicodeSet understands.
PTAL. Most fixed, and comments inlne. > I think it's a great direction for improvement of this code. Thanks, Koji! Thanks! > Maybe I missed some earlier discussion but could you explain the values/unit of the performance figures you're presenting? What subset of codepoints is used? I believe there are good improvements, but the percentage values are a bit hard to interpret for me without the context of what is being measured. The tests were in up to PS5, link here: https://codereview.chromium.org/1541393003/diff/80001/third_party/WebKit/Sour... P(All, 0x0000, 0x1FFFFF, 10) P(ASCII, 0x0020, 0x007E, 100000) P(Han, 0x4E00, 0x9FFF, 1000) P(Hira, 0x3041, 0x3093, 10000) P(Arabic, 0x0600, 0x06FF, 10000) > See below for my concern regarding the ad-hoc large temporary allocation. It'd be great if we can avoid set, either by fusing our range information here differently, or perhaps reconsidering UnicodeSet to avoid writing too much code for this purpose. > Did you compare the performance of UnicodeSet to UTrie2? Commented inline, but there's a clear win in UTrie2 for run-time memory. Didn't really compare the perf yet, but if UnicodeSet is faster, why does uscript_getScript() and all other ICU functions use UTrie2? https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/Character.cpp (right): https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:38: #define MUTEX_H On 2016/01/14 12:10:53, drott wrote: > Why is this needed? ICU fails to compile without this, at least on Windows. I'll investigate a bit more if this is because of our build environment or a general bug in ICU, and if the latter, post to ICU ML. https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:98: 0x2672, 0x267D, On 2016/01/14 12:10:53, drott wrote: > Could you perhaps include > > // Emoji HEAVY HEART EXCLAMATION MARK ORNAMENT..HEAVY BLACK HEART > // Needed in order not to break Emoji heart-kiss sequences in > // CachingWordShapeIterator. > // cmp. http://www.unicode.org/emoji/charts/emoji-zwj-sequences.html > 0x2763, 0x2764, > > here already in order to avoid conflicts with my emoji ZWJ sequences patch? > Done. https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:100: // Ideographic Description Characters, with CJK Symbols and Punctuation, excluding 0x3030. On 2016/01/14 12:10:53, drott wrote: > I know that this is previous code in a way. However, as we're touching it here > and Blink is converging with Chromium's style. Could you ensure an 80 column > limit in the whole file? Wasn't aware that 80 column limit is also coming. Done for all changed lines. Didn't bother with lines without changes, sorry, because Character::characterRangeCodePath() is going to be removed soon anyway. https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:125: // UNICODE VERTICAL TEXT LAYOUT http://www.unicode.org/reports/tr50/ On 2016/01/14 12:10:53, drott wrote: > I wouldn't uppercase the comment here. Perhaps: > > // Individual codepoints < 0xFF from > // needed for Unicode vertical text layout according to > // http://www.unicode.org/reports/tr50/ > // Taken from the corresponding data file: > // http://www.unicode.org/Public/vertical/revision-15/VerticalOrientation-15.txt > Done (actually data is still rev 13, I need to update to 15...) https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:268: const UChar32 maxCodePointForPropertyValues = 0x10FFFD; On 2016/01/14 12:10:53, drott wrote: > As far as I can see this is only used for the assertions and the dependency to > the 0x10FFFD value in isUprightInMixedVerticalRanges is not documented or clear > here. I would find it clearer to not redefine a value here and use kMaxCodepoint > from CharacterNames and use that for the ASSERTions. Couldn't find that definition, thanks! https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Character.cpp:270: static void setRanges(CharacterProperties* values, const UChar32* ranges, size_t length, CharacterProperties value) On 2016/01/14 12:10:53, drott wrote: > If I understand correctly, the CharacterProperties array is a rather heavy 8,5MB > temporary data structure to fuse the two characteristics isCJKIdeographOrSymbol > and isUprightInMixedVertical. I'm a little wary of this large ad-hoc allocation > that can be avoided. > > It could be faster (because gaps could be skipped) and leaner in terms of memory > if we had two iterators that could find the next nearest boundary for > isCJKIdeographOrSymbol and isUprightInMixedVertical, keep track of the mode of > the property values (i.e. 0x01, 0x02 or 0x03) and call utrie2_setRange32 > accordingly. > > To simplify, isolated codepoints could be defined as trivial > one-codepoint-ranges, too. > > If that's too much code to write, UnicodeSet probably does this work for us if > we define the ranges in the pattern format that UnicodeSet understands. It's 1.1MB because CharacterProperties is uint8_t. Is it still too much? I made this simple because: * It's 1.1MB, and only alive within this function. * Hoping to get this function out to build time, though it may take a bit (learned gn, but gyp looks like harder. My colleague recommended to do it after moving to gn. Maybe gn-only is good for now, as it helps Android.) * As we measure, we may want more methods to move to this. For instance, Character:: treatAsZeroWidthSpace() https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... could also move to this. Iterator approach can be more complex and slower if we add more. With UnicodeSet, we need one UnicodeSet for each property, so true we don't need this, but we need more memory in run time. One UTrie2 can hold up to 16/32 bits value, so it's more effective once it's built, but to build, we need the merged value. WDYT?
> > Maybe I missed some earlier discussion but could you explain the values/unit > of > the performance figures you're presenting? What subset of codepoints is used? I > believe there are good improvements, but the percentage values are a bit hard to > interpret for me without the context of what is being measured. > > The tests were in up to PS5, link here: > https://codereview.chromium.org/1541393003/diff/80001/third_party/WebKit/Sour... > P(All, 0x0000, 0x1FFFFF, 10) > P(ASCII, 0x0020, 0x007E, 100000) > P(Han, 0x4E00, 0x9FFF, 1000) > P(Hira, 0x3041, 0x3093, 10000) > P(Arabic, 0x0600, 0x06FF, 10000) Thanks, and the unit is milliseconds? Measured with $ time blink_platform_unitttests...? > > Did you compare the performance of UnicodeSet to UTrie2? > > Commented inline, but there's a clear win in UTrie2 for run-time memory. Didn't > really compare the perf yet, but if UnicodeSet is faster, why does > uscript_getScript() and all other ICU functions use UTrie2? Okay, the fact that we can merge the properties into one trie is convincing. > https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/fonts/Character.cpp (right): > > https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/fonts/Character.cpp:38: #define MUTEX_H > On 2016/01/14 12:10:53, drott wrote: > > Why is this needed? > > ICU fails to compile without this, at least on Windows. I'll investigate a bit > more if this is because of our build environment or a general bug in ICU, and if > the latter, post to ICU ML. Thanks, yes, let's find out what's going on. > Done for all changed lines. Didn't bother with lines without changes, sorry, > because Character::characterRangeCodePath() is going to be removed soon anyway. Sure. > https://codereview.chromium.org/1541393003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/fonts/Character.cpp:270: static void > setRanges(CharacterProperties* values, const UChar32* ranges, size_t length, > CharacterProperties value) > On 2016/01/14 12:10:53, drott wrote: > > If I understand correctly, the CharacterProperties array is a rather heavy > 8,5MB > > temporary data structure to fuse the two characteristics > isCJKIdeographOrSymbol > > and isUprightInMixedVertical. I'm a little wary of this large ad-hoc > allocation > > that can be avoided. > > > > It could be faster (because gaps could be skipped) and leaner in terms of > memory > > if we had two iterators that could find the next nearest boundary for > > isCJKIdeographOrSymbol and isUprightInMixedVertical, keep track of the mode of > > the property values (i.e. 0x01, 0x02 or 0x03) and call utrie2_setRange32 > > accordingly. > > > > To simplify, isolated codepoints could be defined as trivial > > one-codepoint-ranges, too. > > > > If that's too much code to write, UnicodeSet probably does this work for us if > > we define the ranges in the pattern format that UnicodeSet understands. > It's 1.1MB because CharacterProperties is uint8_t. Is it still too much? I made > this simple because: > * It's 1.1MB, and only alive within this function. True, I miscounted somehow, I multiplied with 8 there, confused bz the uint8_t (－‸ლ) >> Could you perhaps include >> // Emoji HEAVY HEART EXCLAMATION MARK ORNAMENT..HEAVY BLACK HEART >> here already in order to avoid conflicts with my emoji ZWJ sequences patch? >> > Done. Could you add this to the tested ranges as well, sorry, I should have mentioned. > * Hoping to get this function out to build time, though it may take a bit > (learned gn, but gyp looks like harder. My colleague recommended to do it after > moving to gn. Maybe gn-only is good for now, as it helps Android.) > * As we measure, we may want more methods to move to this. For instance, > Character:: treatAsZeroWidthSpace() > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > could also move to this. Iterator approach can be more complex and slower if we > add more. Since you're saying the build time construction of the trie might still be a while, I would prefer if we could avoid the temporary allocation, even if it's way smaller than I assumed. If we simplify the range and isolated codepoints definitions to just two range arrays for the two properties we return, and keep them sorted, we could keep just keep one cursor and increment the array indices in an alternating way, compare where is the next nearest and write straight to the trie. I think, given that for additional dimensions the values are currently read, OR'ed and stored back as well, the proportional performance impact of adding a third dimension like treatAsZeroWidthSpace is the same in the two approaches. The complexity seems manageable and we have test cases already to check whether everything will work as expected. > With UnicodeSet, we need one UnicodeSet for each property, so true we don't need > this, but we need more memory in run time. One UTrie2 can hold up to 16/32 bits > value, so it's more effective once it's built, but to build, we need the merged > value. Yes, UTrie2 looks like the way to go.
On 2016/01/14 17:12:12, drott wrote: > > P(All, 0x0000, 0x1FFFFF, 10) > > P(ASCII, 0x0020, 0x007E, 100000) > > P(Han, 0x4E00, 0x9FFF, 1000) > > P(Hira, 0x3041, 0x3093, 10000) > > P(Arabic, 0x0600, 0x06FF, 10000) > > Thanks, and the unit is milliseconds? Measured with $ time > blink_platform_unitttests...? Yes, and blink_platform_unittests displays how long each test took to run. I didn't look into how gtest measures the numbers, but the numbers are quite consistent across runs, so I think it's reliable. > >> Could you perhaps include > >> // Emoji HEAVY HEART EXCLAMATION MARK ORNAMENT..HEAVY BLACK HEART > >> here already in order to avoid conflicts with my emoji ZWJ sequences patch? > >> > > > Done. > > Could you add this to the tested ranges as well, sorry, I should have mentioned. will do, but maybe your Cl goes first. > Since you're saying the build time construction of the trie might still be a > while, I would prefer if we could avoid the temporary allocation, even if it's > way smaller than I assumed. If we simplify the range and isolated codepoints > definitions to just two range arrays for the two properties we return, and keep > them sorted, we could keep just keep one cursor and increment the array indices > in an alternating way, compare where is the next nearest and write straight to > the trie. I think, given that for additional dimensions the values are currently > read, OR'ed and stored back as well, the proportional performance impact of > adding a third dimension like treatAsZeroWidthSpace is the same in the two > approaches. The complexity seems manageable and we have test cases already to > check whether everything will work as expected. I'm thinking to put this on hold. Yes it's doable, but efforts are more useful to spend on build-time construction rather than optimizing this function. That was actually the original plan, but I saw you're building in run-time, and experiments shows that, if similar iterations occur as in the tests, run-time building is still a huge win, so I changed mind to land it first in this CL. Now concerns are raised, so back to the original plan to land when build-time construction is done looks more reasonable to me. After some thinking, doing full automatic building may take longer to complete, but half-manual approach might be able to land earlier. Thank you for the review anyway, that helped me to think further. I'll get back when build-time construction is ready for review.
Description was changed from
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster, this time by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). Since UTrie2 is the data
structure ICU property functions use, the performance of the function
is now as fast as the ICU functions.
In addition in this patch:
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
Ideally we should load the UTrie2 from a serialized frozen byte array
which can be built during the build time. It should be in a future patch
when I figure out how to do it, but the benefit to switch is obvious
without that work.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
to
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster, this time by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] made it faster by ~90% for codepoints < U+2020, but
codepoints abvoe U+2020 were not as fast. This CL makes all codepoints
faster, as fast as ICU functions.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). This patch switches to use
UTrie2, which is the data structure ICU property functions use.
In addition in this patch:
* U+2763 and U+2764 are added as requested by drott@.
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
Description was changed from
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster, this time by using UTrie2, a trie tree in ICU.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] made it faster by ~90% for codepoints < U+2020, but
codepoints abvoe U+2020 were not as fast. This CL makes all codepoints
faster, as fast as ICU functions.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). This patch switches to use
UTrie2, which is the data structure ICU property functions use.
In addition in this patch:
* U+2763 and U+2764 are added as requested by drott@.
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
to
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster.
The previous CL[1] made it faster by ~90% for codepoints below U+2020,
but codepoints abvoe U+2020 were not as fast. This CL makes all
codepoints faster, as fast as ICU functions.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). This patch changes to use
UTrie2, which is the data structure ICU property functions use.
In addition in this patch:
* U+2763 and U+2764 are added as requested by drott@.
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
PTAL. Back from on-hold with a new (a bit wild) idea that popped in my head. No generators are written in C++ today, so making build script was a concern. The idea is to define the generator in gtest and devs need to run the generator manually. But as long as we run it after changing the data, there's no run-time cost. Does this look reasonable?
Oh BTW, this will conflict with your CL I just replied. Please land first and I'll merge.
One more thing: the ICU compile failure on Windows was reported to: http://bugs.icu-project.org/trac/ticket/12075
drott@chromium.org changed reviewers: + dpranke@chromium.org
On 2016/01/15 08:00:41, kojii wrote: > PTAL. Back from on-hold with a new (a bit wild) idea that popped in my head. > > No generators are written in C++ today, so making build script was a concern. > The idea is to define the generator in gtest and devs need to run the generator > manually. But as long as we run it after changing the data, there's no run-time > cost. It's great to see that we're moving towards build time. I've CC'ed Dirk and Paweł on the bug to discuss how this should go into the build system. Perhaps they have some good ideas that would help us here.
On 2016/01/15 10:00:38, drott wrote: > On 2016/01/15 08:00:41, kojii wrote: > > PTAL. Back from on-hold with a new (a bit wild) idea that popped in my head. > > > > No generators are written in C++ today, so making build script was a concern. > > The idea is to define the generator in gtest and devs need to run the > generator > > manually. But as long as we run it after changing the data, there's no > run-time > > cost. > > It's great to see that we're moving towards build time. I've CC'ed Dirk and > Paweł on the bug to discuss how this should go into the build system. Perhaps > they have some good ideas that would help us here. Great thank you. ...can we make it a separate work? I'd like to parallelize build system work and other Character-related work.
> Great thank you. > > ...can we make it a separate work? I'd like to parallelize build system work and > other Character-related work. Sorry for the delay in continuing the review. I took a step back and tried to thoroughly consider what we should do here. Let me again say that I really appreciate the effort of improving the character property functions. We can really profit from that and your CL shows that it's generally doable and brings great advantages. For integrating it properly, I think it's also clear that we should perform the trie construction at build time. However, I am convinced we can not leave the tree in an interim state where there is no automatic way of regenerating the trie header file. In a way, this would be a regression in our ability to touch the range definitions and modify our own code. In my opinion, the approach using a gtest for the trie generation is a surprising shortcut, and it's not in the right place given its purpose. If such a generator tool is not regularly run as part of the build process, it becomes prone to bit rot. In other words, I think we can not separate landing the preconstructed trie and landing a change to the build system which regularly runs the generator to output and compile the serialized utrie data. This ensure that the tool stays maintained and breaks the bots if it fails to run. Now, I've been thinking about two ways of achieving that. I've found two examples of binaries run as part of the build process which may help finding the right approach: 1) tools/android/md5sum/ builds an md5sum tool that is later used in the build process to produce additional output. 2) In the gyp build process there is a proto_library() primitive that is used to invoce protoc_wrapper.py, which invokes the protobuf compiler binary to generate output in the build out/ directories. And there's a separate build target that builds the protoc compiler prior to that. I think these are two examples of doing something similar to what we have in mind for the trie generation. If it's easier, perhaps another potential approach is to write a Python script using ctypes to bind to the icu .so and construct the serialized utrie with that, then integrate this script to the build system. I hope this helps with finding the right way to hook up the trie generator into the build system. Again, thanks for working on this improvement. The proof that it works and brings improvements is certainly made, now let's get it into the tree in the right way.
Thank you for finding examples, I can have a look, but I don't know why you care one manual process so much. It's not an interim state, it just doesn't happen at all, and we have some such examples already. W3C imports are completely manual process after import specs are updated. There are some such in UMA too. Thought about python approach, but to make sure the right ICU (not in the system) to load needs a bit of study for me. Anyway, I might look into them, but could be slow. If you changed your mind by knowing there are other such examples, please comment so.
On 2016/01/18 10:49:26, kojii wrote: > Thank you for finding examples, I can have a look, but I don't know why you care > one manual process so much. It's not an interim state, it just doesn't happen at > all, and we have some such examples already. W3C imports are completely manual > process after import specs are updated. There are some such in UMA too. I tried to explain why and what I care about in my previous response: Most importantly, there should be no additional burden for someone making changes to blink layout code other than editing source files, since it slows down the developer coming across this situation. Automation can take care of the rest and allows scale. If we have a tool that is not regularly built, it's likely to break over time because it doesn't have bot coverage and for example ICU API changes may go unnoticed. Plus, gtest is not the right way for building such a tool. Currently, we are able to edit Character.cpp file and we have previously touched the ranges or modified properties. I disagree with the assumption that changes to the range definitions do not happen at all. The ranges have previously changed and we're likely to touch them in the future. We might also add additional properties or migrate existing ones, for example for fusing the emoji properties as well depending on when ICU upstream provides API for it, or when we find out that ranges are missing, or when there changes in Unicode that require us to do so etc.. So, we need the range definitions to be still editable and we cannot practically freeze them by making it harder to update the serialized trie data. The next contributor editing ranges of isCJKIdeographOrSymbol or isUprightInMixedVertical must have straightforward means of making a change that is not harder than editing a file (whether that's a Python or a C-file) and automation steps in the build system must take care of the rest. I do not know of a precedent for adding manual code generation steps to editing functional code in Blink, and I don't think we should introduce one here. I consider manual test imports a separate issue.
I didn't mean ranges will not change, I agree it will. I meant when dev forgot to run the tool, the change will not appear in the build at all, not "leave the tree in an interim state." Make sense? I probably failed to say I acknowledged your opinion, sorry about that. I still think differently, but your opinion is honored.
Asked around a few experts on generators, here's short summary:
1. There are both cases where we generate on build, and where we commit
generated files. It's up to the team.
2. Python+ctype won't help much since it needs to build ICU anyway for possible
version mismatch and for platforms lacking ICU (e.g., win)
3. If we chose to generate on build, it must take care of cross-compiler cases
(use different set of compilers for generator and target.)
4. Samples to look at:
- protobuf builds protoc and then run it on build time, so a good sample to
use different compiler for cross-compile case.
- webrtc/base/bind.h is where generated source is committed.
- protoc uses rules, but this case input/output is clear, binding is probably
easier to look at for adding actions to gyp:
- WebKit/Source/bindings/core/v8/generated.gyp
- WebKit/Source/build/scripts/
5. brettw@ may be able to advice for GN if any.
6. Another option is to build the generator and upload the binary.
dpranke@, could you mind to review gyp-cross-compile part? Among the combinations of: 1. gn, self-compile 2. gyp, self-compile 3. gn, cross-compile 4. gyp, cross-compile I think 1-3 are working, but "gyp, cross-compile" fails. Searched "RemoveLinkDependenciesFromNoneTargets" but didn't help much. I understand Android is moving to gn sometime soon (?) http://crbug.com/576993, but chromeos still remains "gyp, cross-compile" combination.
The code (including the GYP changes) looks correct to me, so I'm not sure why you're getting the errors you're getting. Then again, I'm not all that familiar with the ICU build, nor with the cros build, so I'm not sure what GYP_DEFINES are getting set and whether that might be affecting something. It would perhaps be interesting to see if you get the same compile failures on the linux_chromium_chromeos_compile_rel_ng bot (which uses a normal linux build w/ the chromeos GYP_DEFINES). Otherwise someone might have to reproduce this locally and dig into what's going on.
On 2016/01/26 23:05:07, Dirk Pranke wrote: > The code (including the GYP changes) looks correct to me, so I'm not sure why > you're getting the errors you're getting. > > Then again, I'm not all that familiar with the ICU build, nor with the cros > build, so I'm not sure what GYP_DEFINES are > getting set and whether that might be affecting something. > > It would perhaps be interesting to see if you get the same compile failures on > the linux_chromium_chromeos_compile_rel_ng bot > (which uses a normal linux build w/ the chromeos GYP_DEFINES). > > Otherwise someone might have to reproduce this locally and dig into what's going > on. Thank you Dirk for your review. I briefly talked with eae@, since we expect not too long (?) until we switch both Android and CrOS to GN, it probably makes sense to go to check-in generated files for now, and integrate to the build process once the switch is done. I filed crbug.com/571943 and will revert this CL to PS11 for review.
eae@, PTAL while drott@ is off?
Looks good but I'd still like to see the generator hooked up to a build target for at least one platform.
On 2016/01/27 02:25:18, eae wrote: > Looks good but I'd still like to see the generator hooked up to a build target > for at least one platform. Ah, I misunderstood that part, sorry. I'll work on that in the next patch.
No worries, I should have been more explicit about that. Thank you.
PTAL. GYP is manual if Android || CrOS || cross-compile. GN is always automatic.
LGTM Thank you!
Thank you for your advice!
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1541393003/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1541393003/460001
Message was sent while issue was closed.
Description was changed from
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster.
The previous CL[1] made it faster by ~90% for codepoints below U+2020,
but codepoints abvoe U+2020 were not as fast. This CL makes all
codepoints faster, as fast as ICU functions.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). This patch changes to use
UTrie2, which is the data structure ICU property functions use.
In addition in this patch:
* U+2763 and U+2764 are added as requested by drott@.
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
to
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster.
The previous CL[1] made it faster by ~90% for codepoints below U+2020,
but codepoints abvoe U+2020 were not as fast. This CL makes all
codepoints faster, as fast as ICU functions.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). This patch changes to use
UTrie2, which is the data structure ICU property functions use.
In addition in this patch:
* U+2763 and U+2764 are added as requested by drott@.
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster.
The previous CL[1] made it faster by ~90% for codepoints below U+2020,
but codepoints abvoe U+2020 were not as fast. This CL makes all
codepoints faster, as fast as ICU functions.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). This patch changes to use
UTrie2, which is the data structure ICU property functions use.
In addition in this patch:
* U+2763 and U+2764 are added as requested by drott@.
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
[1] https://codereview.chromium.org/1545073002
BUG=571943
==========
to
==========
Improve performance of Character::isCJKIdeographOrSymbol by using trie tree
This patch is another effort to make Character::isCJKIdeographOrSymbol
faster.
The previous CL[1] made it faster by ~90% for codepoints below U+2020,
but codepoints abvoe U+2020 were not as fast. This CL makes all
codepoints faster, as fast as ICU functions.
Before After Improve ICU
All 2569 => 292 88% 298
ASCII 68 => 68 0% 160
Han 2958 => 263 91% 344
Hira 258 => 11 95% 14
Arabic 37 => 32 13% 44
* # of code points and iterations vary by rows.
The previous CL[1] clarified that binary search is not as fast as ICU
functions such as uscript_getScript(). This patch changes to use
UTrie2, which is the data structure ICU property functions use.
In addition in this patch:
* U+2763 and U+2764 are added as requested by drott@.
* Character::isUprightInMixedVertical() was switched to UTrie2 too.
* Character::isCJKIdeograph() was removed because it is no longer used.
[1] https://codereview.chromium.org/1545073002
BUG=571943
Committed: https://crrev.com/e6dc3f425137021f39eddbbeb2035273ce36f986
Cr-Commit-Position: refs/heads/master@{#371917}
==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/e6dc3f425137021f39eddbbeb2035273ce36f986 Cr-Commit-Position: refs/heads/master@{#371917}
Message was sent while issue was closed.
On 2016/01/28 00:16:28, commit-bot: I haz the power wrote: > Patchset 24 (id:??) landed as > https://crrev.com/e6dc3f425137021f39eddbbeb2035273ce36f986 > Cr-Commit-Position: refs/heads/master@{#371917} Local tsan gn builds appear to fail due to this change: [90/13309] LINK ./character_data_generator FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings -fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -B../../third_party/binutils/Linux_x64/Release/bin -fuse-ld=gold -pthread -m64 -Wl,-O1 -Wl,--gc-sections --sysroot=/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot -L/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/lib/x86_64-linux-gnu\ -Wl,-rpath-link=/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/lib/x86_64-linux-gnu\ -L/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu\ -Wl,-rpath-link=/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/x86_64-linux-gnu\ -L/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6\ -Wl,-rpath-link=/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6\ -L/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib\ -Wl,-rpath-link=/usr/local/google/home/amistry/chrome-source/src/build/linux/debian_wheezy_amd64-sysroot/usr/lib -Wl,-rpath-link=../Release.gn -Wl,--disable-new-dtags -Wl,-rpath=\$ORIGIN/. -Wl,-rpath-link=. -o "./character_data_generator" -Wl,--start-group @"./character_data_generator.rsp" ./libicui18n.so ./libicuuc.so -Wl,--end-group -ldl -lrt ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:271: error: undefined reference to '__tsan_func_entry' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_unaligned_read16' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_unaligned_write16' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_unaligned_read16' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_unaligned_write16' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_unaligned_read16' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_unaligned_write16' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_unaligned_read16' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_unaligned_write16' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:237: error: undefined reference to '__tsan_read4' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:237: error: undefined reference to '__tsan_read4' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_write1' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:237: error: undefined reference to '__tsan_read4' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:249: error: undefined reference to '__tsan_read4' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_write1' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_write1' ../../third_party/WebKit/Source/platform/fonts/CharacterProperty.h:35: error: undefined reference to '__tsan_write1' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:297: error: undefined reference to '__tsan_write4' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:301: error: undefined reference to '__tsan_read1' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:303: error: undefined reference to '__tsan_read1' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:313: error: undefined reference to '__tsan_read1' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:320: error: undefined reference to '__tsan_write4' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:327: error: undefined reference to '__tsan_read8' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:329: error: undefined reference to '__tsan_read8' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:337: error: undefined reference to '__tsan_func_exit' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:255: error: undefined reference to '__tsan_func_entry' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:263: error: undefined reference to '__tsan_read1' ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:268: error: undefined reference to '__tsan_func_exit' obj/third_party/WebKit/Source/platform/character_data_generator/CharacterDataGenerator.o:../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:function tsan.module_ctor: error: undefined reference to '__tsan_init'
Message was sent while issue was closed.
On 2016/01/28 03:13:13, Anand Mistry wrote: > On 2016/01/28 00:16:28, commit-bot: I haz the power wrote: > > Patchset 24 (id:??) landed as > > https://crrev.com/e6dc3f425137021f39eddbbeb2035273ce36f986 > > Cr-Commit-Position: refs/heads/master@{#371917} > > Local tsan gn builds appear to fail due to this change: Sorry for the trouble, are you using gyp or gn? Haven't used tsan before but I can try to look into it.
Message was sent while issue was closed.
On 2016/01/28 03:17:15, kojii wrote: > On 2016/01/28 03:13:13, Anand Mistry wrote: > > On 2016/01/28 00:16:28, commit-bot: I haz the power wrote: > > > Patchset 24 (id:??) landed as > > > https://crrev.com/e6dc3f425137021f39eddbbeb2035273ce36f986 > > > Cr-Commit-Position: refs/heads/master@{#371917} > > > > Local tsan gn builds appear to fail due to this change: > > Sorry for the trouble, are you using gyp or gn? Haven't used tsan before but I > can try to look into it. Oh you said "gn", I'm trying...
Message was sent while issue was closed.
A revert of this CL (patchset #24 id:460001) has been created in https://codereview.chromium.org/1644893002/ by kojii@chromium.org. The reason for reverting is: GN asan and tsan are broken, wasn't aware bots are running GYP only. Reverting until we can figure out what to do with GN asan/tsan and probably msan..
Message was sent while issue was closed.
Also fail build if use -Duse_system_icu=1 and newer version of ICU (for example ICU 56)
Message was sent while issue was closed.
On 2016/01/30 09:49:24, sL1pKn07 wrote: > Also fail build if use -Duse_system_icu=1 and newer version of ICU (for example > ICU 56) [1531/19105] /tmp/makepkg/chromium-dev/src/chromium-50.0.2633.3/third_party/llvm-build/Release+Asserts/bin/clang++ -Qunused-arguments -MMD -MF obj/third_party/sfntly/src/cpp/src/sfntly/table/bitmap/sfntly.index_sub_table_format2.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=257955-1 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_X11=1 -DUSE_CLIPBOARD_AURAX11=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_TOPCHROME_MD=1 -DUSE_UDEV -DDONT_EMBED_BUILD_METADATA -DFIELDTRIAL_TESTING_ENABLED -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_HANGOUT_SERVICES_EXTENSION=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DSFNTLY_NO_EXCEPTION -DU_USING_ICU_NAMESPACE=0 -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -Igen/shim_headers/icuuc/target -Igen -I../../third_party/sfntly/src/cpp/src -fstack-protector --param=ssp-buffer-size=4 -pthread -fno-strict-aliasing -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Xclang -load -Xclang /tmp/makepkg/chromium-dev/src/chromium-50.0.2633.3/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-templates -fcolor-diagnostics -Wheader-hygiene -Wfor-loop-analysis -Wno-char-subscripts -Wno-unneeded-internal-declaration -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register -Wno-inconsistent-missing-override -Wno-shift-negative-value -m64 -march=x86-64 -O2 -fno-ident -fdata-sections -ffunction-sections -funwind-tables -march=native -O2 -pipe -fstack-protector-strong -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -c ../../third_party/sfntly/src/cpp/src/sfntly/table/bitmap/index_sub_table_format2.cc -o obj/third_party/sfntly/src/cpp/src/sfntly/table/bitmap/sfntly.index_sub_table_format2.o FAILED: /tmp/makepkg/chromium-dev/src/chromium-50.0.2633.3/third_party/llvm-build/Release+Asserts/bin/clang++ -Qunused-arguments -MMD -MF obj/third_party/WebKit/Source/platform/fonts/character_data_generator.CharacterDataGenerator.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=257955-1 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_LIBJPEG_TURBO=1 -DUSE_X11=1 -DUSE_CLIPBOARD_AURAX11=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_TOPCHROME_MD=1 -DUSE_UDEV -DDONT_EMBED_BUILD_METADATA -DFIELDTRIAL_TESTING_ENABLED -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_HANGOUT_SERVICES_EXTENSION=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DU_USING_ICU_NAMESPACE=0 -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -Igen/shim_headers/icuuc/target -Igen -fstack-protector --param=ssp-buffer-size=4 -pthread -fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -Xclang -load -Xclang /tmp/makepkg/chromium-dev/src/chromium-50.0.2633.3/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -add-plugin -Xclang find-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang check-templates -fcolor-diagnostics -Wheader-hygiene -Wfor-loop-analysis -Wno-char-subscripts -Wno-unneeded-internal-declaration -Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register -Wno-inconsistent-missing-override -Wno-shift-negative-value -Wno-unused-variable -m64 -march=x86-64 -O2 -fno-ident -fdata-sections -ffunction-sections -funwind-tables -march=native -O2 -pipe -fstack-protector-strong -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -std=gnu++11 -Wno-deprecated -c ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp -o obj/third_party/WebKit/Source/platform/fonts/character_data_generator.CharacterDataGenerator.o ../../third_party/WebKit/Source/platform/fonts/CharacterDataGenerator.cpp:12:10: fatal error: 'utrie2.h' file not found |
