Skip to content

GitLab

T
tangled
Commit 3aad1896 authored by kojii's avatar kojii Committed by Commit bot
Browse files
Options

Revert of Improve performance of Character::isCJKIdeographOrSymbol by using... 

Revert of Improve performance of Character::isCJKIdeographOrSymbol by using trie tree (patchset #24 id:460001 of https://codereview.chromium.org/1541393003/ )

Reason for revert:
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.

Original issue's description:
> 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}

TBR=eae@chromium.org,drott@chromium.org,dpranke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=571943

Review URL: https://codereview.chromium.org/1644893002

Cr-Commit-Position: refs/heads/master@{#372004}
parent 4e4c1c07
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 253 additions and 1146 deletions
third_party/WebKit/Source/platform/BUILD.gn
View file @ 3aad1896
...@@ -139,38 +139,6 @@ action("color_data") { ...@@ -139,38 +139,6 @@ action("color_data") {
] ]
} }
action("character_data") {
script = "../build/scripts/gperf.py"
deps = [
":character_data_generator($host_toolchain)",
]
output_file = "$blink_platform_output_dir/CharacterData.cpp"
outputs = [
output_file,
]
# Find character_data_generator, which is generated in a different directory
# when cross-compile.
generator = "./" + rebase_path(
get_label_info(":character_data_generator($host_toolchain)",
"root_out_dir") + "/character_data_generator",
root_build_dir)
args = [
generator,
rebase_path(output_file, root_build_dir),
]
}
executable("character_data_generator") {
sources = [
"fonts/CharacterDataGenerator.cpp",
]
configs += [ "//third_party/WebKit/Source:config" ]
deps = [
"//third_party/icu",
]
}
# This isn't strictly necessary since we can just add the deps to "platform", # This isn't strictly necessary since we can just add the deps to "platform",
# but it helps to have the targets match the GYP build. # but it helps to have the targets match the GYP build.
group("make_platform_generated") { group("make_platform_generated") {
...@@ -178,7 +146,6 @@ group("make_platform_generated") { ...@@ -178,7 +146,6 @@ group("make_platform_generated") {
visibility = [ "//third_party/WebKit/Source/*" ] visibility = [ "//third_party/WebKit/Source/*" ]
public_deps = [ public_deps = [
":blink_common", ":blink_common",
":character_data",
":color_data", ":color_data",
":font_family_names", ":font_family_names",
":http_names", ":http_names",
...@@ -198,11 +165,10 @@ component("platform") { ...@@ -198,11 +165,10 @@ component("platform") {
sources -= blink_platform_sse_files sources -= blink_platform_sse_files
# Add in the generated files. # Add in the generated files.
sources += sources += get_target_outputs(":font_family_names") +
get_target_outputs(":font_family_names") + get_target_outputs(":http_names") +
get_target_outputs(":http_names") + get_target_outputs(":runtime_enabled_features") +
get_target_outputs(":runtime_enabled_features") + get_target_outputs(":color_data")
get_target_outputs(":color_data") + get_target_outputs(":character_data")
configs += [ configs += [
# TODO(jschuh): crbug.com/167187 fix size_t to int truncations. # TODO(jschuh): crbug.com/167187 fix size_t to int truncations.
......
third_party/WebKit/Source/platform/blink_platform.gyp
View file @ 3aad1896
...@@ -177,7 +177,6 @@ ...@@ -177,7 +177,6 @@
'<@(platform_heap_files)', '<@(platform_heap_files)',
# Additional .cpp files from platform_generated.gyp:make_platform_generated actions. # Additional .cpp files from platform_generated.gyp:make_platform_generated actions.
'<(blink_platform_output_dir)/CharacterData.cpp',
'<(blink_platform_output_dir)/ColorData.cpp', '<(blink_platform_output_dir)/ColorData.cpp',
'<(blink_platform_output_dir)/FontFamilyNames.cpp', '<(blink_platform_output_dir)/FontFamilyNames.cpp',
'<(blink_platform_output_dir)/HTTPNames.cpp', '<(blink_platform_output_dir)/HTTPNames.cpp',
......
third_party/WebKit/Source/platform/fonts/Character.h
View file @ 3aad1896
...@@ -32,7 +32,6 @@ ...@@ -32,7 +32,6 @@
#define Character_h #define Character_h
#include "platform/PlatformExport.h" #include "platform/PlatformExport.h"
#include "platform/fonts/CharacterProperty.h"
#include "platform/text/TextDirection.h" #include "platform/text/TextDirection.h"
#include "platform/text/TextPath.h" #include "platform/text/TextPath.h"
#include "platform/text/TextRun.h" #include "platform/text/TextRun.h"
...@@ -62,6 +61,7 @@ public: ...@@ -62,6 +61,7 @@ public:
|| isInRange(character, 0xE0100, 0xE01EF); // VARIATION SELECTOR-17 to 256 || isInRange(character, 0xE0100, 0xE01EF); // VARIATION SELECTOR-17 to 256
} }
static bool isCJKIdeograph(UChar32);
static bool isCJKIdeographOrSymbol(UChar32); static bool isCJKIdeographOrSymbol(UChar32);
static unsigned expansionOpportunityCount(const LChar*, size_t length, TextDirection, bool& isAfterExpansion, const TextJustify); static unsigned expansionOpportunityCount(const LChar*, size_t length, TextDirection, bool& isAfterExpansion, const TextJustify);
...@@ -132,8 +132,6 @@ public: ...@@ -132,8 +132,6 @@ public:
static bool isCommonOrInheritedScript(UChar32); static bool isCommonOrInheritedScript(UChar32);
private:
static bool hasProperty(UChar32, CharacterProperty);
}; };
} // namespace blink } // namespace blink
......
third_party/WebKit/Source/platform/fonts/CharacterProperty.h deleted 100644 → 0
View file @ 4e4c1c07
// Copyright 2016 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include <cstdint>
namespace blink {
using CharacterPropertyType = uint8_t;
enum class CharacterProperty : CharacterPropertyType {
isCJKIdeographOrSymbol = 0x0001,
isUprightInMixedVertical = 0x0002,
};
inline CharacterProperty operator | (
CharacterProperty a, CharacterProperty b)
{
return static_cast<CharacterProperty>(
static_cast<CharacterPropertyType>(a)
| static_cast<CharacterPropertyType>(b));
}
inline CharacterProperty operator & (
CharacterProperty a, CharacterProperty b)
{
return static_cast<CharacterProperty>(
static_cast<CharacterPropertyType>(a)
& static_cast<CharacterPropertyType>(b));
}
inline CharacterProperty operator |= (
CharacterProperty& a, CharacterProperty b)
{
a = a | b;
return a;
}
} // namespace blink
third_party/WebKit/Source/platform/fonts/CharacterTest.cpp
View file @ 3aad1896
...@@ -143,17 +143,13 @@ TEST(CharacterTest, TestCharacterRangeCodePathString) ...@@ -143,17 +143,13 @@ TEST(CharacterTest, TestCharacterRangeCodePathString)
EXPECT_EQ(ComplexPath, Character::characterRangeCodePath(c6, 3)); EXPECT_EQ(ComplexPath, Character::characterRangeCodePath(c6, 3));
} }
static void TestSpecificUChar32RangeIdeograph(UChar32 rangeStart, static void TestSpecificUChar32RangeIdeograph(UChar32 rangeStart, UChar32 rangeEnd)
UChar32 rangeEnd,
bool before = true)
{ {
if (before) EXPECT_FALSE(Character::isCJKIdeograph(rangeStart - 1));
EXPECT_FALSE(Character::isCJKIdeographOrSymbol(rangeStart - 1)); EXPECT_TRUE(Character::isCJKIdeograph(rangeStart));
EXPECT_TRUE(Character::isCJKIdeographOrSymbol(rangeStart)); EXPECT_TRUE(Character::isCJKIdeograph((UChar32)((uint64_t)rangeStart + (uint64_t)rangeEnd) / 2));
EXPECT_TRUE(Character::isCJKIdeographOrSymbol( EXPECT_TRUE(Character::isCJKIdeograph(rangeEnd));
(UChar32)((uint64_t)rangeStart + (uint64_t)rangeEnd) / 2)); EXPECT_FALSE(Character::isCJKIdeograph(rangeEnd + 1));
EXPECT_TRUE(Character::isCJKIdeographOrSymbol(rangeEnd));
EXPECT_FALSE(Character::isCJKIdeographOrSymbol(rangeEnd + 1));
} }
TEST(CharacterTest, TestIsCJKIdeograph) TEST(CharacterTest, TestIsCJKIdeograph)
...@@ -161,11 +157,11 @@ TEST(CharacterTest, TestIsCJKIdeograph) ...@@ -161,11 +157,11 @@ TEST(CharacterTest, TestIsCJKIdeograph)
// The basic CJK Unified Ideographs block. // The basic CJK Unified Ideographs block.
TestSpecificUChar32RangeIdeograph(0x4E00, 0x9FFF); TestSpecificUChar32RangeIdeograph(0x4E00, 0x9FFF);
// CJK Unified Ideographs Extension A. // CJK Unified Ideographs Extension A.
TestSpecificUChar32RangeIdeograph(0x3400, 0x4DBF, false); TestSpecificUChar32RangeIdeograph(0x3400, 0x4DBF);
// CJK Unified Ideographs Extension A and Kangxi Radicals. // CJK Unified Ideographs Extension A and Kangxi Radicals.
TestSpecificUChar32RangeIdeograph(0x2E80, 0x2FDF); TestSpecificUChar32RangeIdeograph(0x2E80, 0x2FDF);
// CJK Strokes. // CJK Strokes.
TestSpecificUChar32RangeIdeograph(0x31C0, 0x31EF, false); TestSpecificUChar32RangeIdeograph(0x31C0, 0x31EF);
// CJK Compatibility Ideographs. // CJK Compatibility Ideographs.
TestSpecificUChar32RangeIdeograph(0xF900, 0xFAFF); TestSpecificUChar32RangeIdeograph(0xF900, 0xFAFF);
// CJK Unified Ideographs Extension B. // CJK Unified Ideographs Extension B.
......
third_party/WebKit/Source/platform/platform_generated.gyp
View file @ 3aad1896
...@@ -34,31 +34,12 @@ ...@@ -34,31 +34,12 @@
'../build/scripts/scripts.gypi', '../build/scripts/scripts.gypi',
'platform_generated.gypi', 'platform_generated.gypi',
], ],
'variables': {
'conditions': [
# TODO(kojii): The character_data_generator fails when cross-compile, so
# we use a pre-generated copy in the tree until we fix or until we move
# to gn. See crbug.com/581555
['OS=="android" or chromeos==1 or (target_arch!="ia32" and target_arch!="x64")', {
'generate_character_data%': 0,
}, {
'generate_character_data%': 1,
}],
],
},
'targets': [ 'targets': [
{ {
'target_name': 'make_platform_generated', 'target_name': 'make_platform_generated',
'type': 'none', 'type': 'none',
'hard_dependency': 1, 'hard_dependency': 1,
'conditions': [
['generate_character_data==1', {
'dependencies': [
'character_data_generator#host',
],
}]
],
'actions': [ 'actions': [
{ {
'action_name': 'FontFamilyNames', 'action_name': 'FontFamilyNames',
...@@ -133,41 +114,7 @@ ...@@ -133,41 +114,7 @@
'--output-file=<(blink_platform_output_dir)/ColorData.cpp', '--output-file=<(blink_platform_output_dir)/ColorData.cpp',
], ],
}, },
{
'action_name': 'CharacterData',
'inputs': [
'fonts/CharacterDataGenerator.cpp',
],
'outputs': [
'<(blink_platform_output_dir)/CharacterData.cpp',
],
'conditions': [
['generate_character_data==1', {
'action': [
'<(PRODUCT_DIR)/character_data_generator',
'<(blink_platform_output_dir)/CharacterData.cpp',
],
}, {
'action': [
'cp',
'fonts/CharacterData.cpp',
'<(blink_platform_output_dir)/CharacterData.cpp',
],
}]
],
},
] ]
}, },
{
'target_name': 'character_data_generator',
'type': 'executable',
'toolsets': ['host'],
'sources': [
'fonts/CharacterDataGenerator.cpp',
],
'dependencies': [
'<(DEPTH)/third_party/icu/icu.gyp:icuuc#host',
],
},
], ],
} }
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment