RunSegmenter::consume() performance |
||||
Issue descriptionDuring the performance investigation of line layout, RunSegmenter::consume() turned out to be not cheap. This bug is to track/discuss possible improvement there.
,
Sep 8 2016
Here's a sample profiling numbers on Win official. When HarfBuzzShaper::shapeResult consumes 71.4% of total time (the test file from issue 617143 ): - shapeRange 31.3% - extractShapeResult 22.6% - consume 10.5% - consumeSymbolsIteratorPastLastSplit 5.2% - SymbolsIterator::fontFallbackPriorityForCharacter 3.7% - isEmojiTextDefault 0.8% - isEmojiEmojiDefault 0.8% - isEmojiEmojiDefault 0.8% - ublock_getCode 0.8% - RunSegmenter::consumeScriptIteratorPastLastSplit 5.2% - fetch 3.3% getScripts 2.3% - getPairedBracketType 1.2% - CaseMappingHarfBuzzBufferFiller 1.7% - Other 5.1% One possible optimization is the 4 functions in fontFallbackPriorityForCharacter each look up Unicode trie or UnicodeSet for the same code point. Maybe we can create a table so that one lookup can do the work? Would it make the maintenance harder?
,
Sep 8 2016
#1: yeah, for scripts, we check many times across different layers. I think there are room to improve there, but haven't got clear idea yet.
,
Sep 8 2016
#1: UTF16Iterator seems to be not bad, only 1 sample (<0.01%) for the constructor and no other functions in the profile, when RunSegmenter::consume recorded 2,718 samples. Maybe win official inlines them.
,
Sep 9 2016
,
Sep 11 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 11 2017
,
Mar 19 2018
While investigating issue 808100, I happened to find some low hanging fruits in vector allocations:
,
Mar 19 2018
We might want to pre-allocate the vector or have a relatively large one shared across invocations.
,
May 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aab641bf58c9e3b17a2ae28804da290bdfb2c17f commit aab641bf58c9e3b17a2ae28804da290bdfb2c17f Author: Koji Ishii <kojii@chromium.org> Date: Wed May 30 18:39:46 2018 Pre-allocate Vector<UScriptCode> in ScriptRunIterator This patch pre-allocates Vector<UScriptCode> in ScriptRunIterator as the allocation of this vector turned out to be one of the hottest code path. Bug: 645035, 846138 Change-Id: Id424d6424d35a9cb7eaf62f15b809843644bd40a Reviewed-on: https://chromium-review.googlesource.com/1077916 Reviewed-by: Dominik Röttsches <drott@chromium.org> Commit-Queue: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#562927} [modify] https://crrev.com/aab641bf58c9e3b17a2ae28804da290bdfb2c17f/third_party/blink/renderer/platform/fonts/script_run_iterator.cc [modify] https://crrev.com/aab641bf58c9e3b17a2ae28804da290bdfb2c17f/third_party/blink/renderer/platform/fonts/script_run_iterator.h [modify] https://crrev.com/aab641bf58c9e3b17a2ae28804da290bdfb2c17f/third_party/blink/renderer/platform/fonts/script_run_iterator_test.cc
,
May 31 2018
Simple question, do we need to collect hint characters for all characters in paragraph?
Yet another profile sample from external/wpt/selection/collapse-30.html
bool HarfBuzzShaper::CollectFallbackHintChars(
const Deque<ReshapeQueueItem>& reshape_queue,
Vector<UChar32>& hint) const {
if (!reshape_queue.size())
return false;
hint.clear();
size_t num_chars_added = 0;
for (auto it = reshape_queue.begin(); it != reshape_queue.end(); ++it) {
if (it->action_ == kReshapeQueueNextFont)
break;
UChar32 hint_char;
CHECK_LE((it->start_index_ + it->num_characters_), text_length_);
UTF16TextIterator iterator(text_ + it->start_index_, it->num_characters_);
5.87% while (iterator.Consume(hint_char)) {
76.27% hint.push_back(hint_char);
5.69% num_chars_added++;
5.07% iterator.Advance();
}
}
return num_chars_added > 0;
}
,
May 31 2018
Interesting point, off the top of my head, the hints are used for finding fallback fonts. If there's none found for the first hint, the next is used, etc. - what does this collapse-30.html test do? Is it very fallback-heavy? This is strictly speaking not a RunSegmenter related profiling result, perhaps we should fork this finding out and look at it under a different bug.
,
May 31 2018
Good point, you're right, it was my bad that the 1st screenshot of #c8 was about fallback, not RunSegmenter, and I told it to yosin without enough thoughts what this bug is about. I'll file a separate bug.
,
May 31 2018
Filed as issue 848295 .
,
Jun 1 2018
>#c12, "collapse-30.html" contains lots of elements and texts, created by script "common.js"[1]. Text contains alphabets, alphabet + accent, Hebrew. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/selection/common.js
,
Jun 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a99dba19ace0afe37014dbb8d7378983fc8c7ab commit 9a99dba19ace0afe37014dbb8d7378983fc8c7ab Author: Koji Ishii <kojii@chromium.org> Date: Fri Jun 01 14:36:24 2018 Make swapping next/ahead_set faster by making them pointers Pre-allocating inline vector for UScriptCode in CL:1077916 improved the overall performance of ScriptRunIterator. In this new code, swapping next_set and ahead_set consumes ~50% of ScriptRunIterator::Fetch(). This part is slower than before because these vectors are now inlined. This patch changes them to pointers to turn the swap operation of inline vectors to the pointer swap operation. This improves perf_tests/layout/word-break-break-all.html running on LayoutNG by ~50%, and reduces ScriptRunIterator::Fetch() to ~25% of total time, and 80% of Fetch() is spent in GetScript(). Note, LayoutNG is still slower than legacy by ~10x on the test, and 63% of the time is in RunSegmenter::ConsumePast(). We will need some more tuning around this. 63% is, though, down from ~90%, with these fixes and a few other WIP fixes. Bug: 645035, 846138 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng Change-Id: I45a71009447015d44ec0ae0ab05dd2621f07c262 Reviewed-on: https://chromium-review.googlesource.com/1080467 Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#563624} [modify] https://crrev.com/9a99dba19ace0afe37014dbb8d7378983fc8c7ab/third_party/blink/renderer/platform/fonts/script_run_iterator.cc [modify] https://crrev.com/9a99dba19ace0afe37014dbb8d7378983fc8c7ab/third_party/blink/renderer/platform/fonts/script_run_iterator.h
,
Jun 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06e705c947d8a481412c9a87c83b862469ef3929 commit 06e705c947d8a481412c9a87c83b862469ef3929 Author: Koji Ishii <kojii@chromium.org> Date: Tue Jun 05 05:11:44 2018 [LayoutNG] Segment NGInlineItem by RunSegmenter This patch segments NGInlineItem using RunSegmenter in SegmentText phase, and let HarfBuzzShaper to skip running RunSegmenter. Because RunSegmenter requires forward only, sequential access to the whole paragraph, it is not cheap when ShapingLineBreaker re-shape each line start and end, especially for large block. By storing RunSegmenter results in NGInlineItem, HarfBuzzShaper no longer runs RunSegmenter when ShapingLineBreaker shapes. Text dumps change because NGPhysicalTextFragment is created for each NGInlineItem. A few CJK and bidi images are also rebaselined due to 1px differences in glyph positions. This patch implements the option 4 of the design doc: https://docs.google.com/document/d/1iGXYED3FPpMLUCUoiOGBvZK9HhCZ2lWm0WSZUClj2gQ/edit?usp=sharing ~10 tests turn to pass with this patch. With this patch, word-break-break-all.html shows quite different results on Windows and Linux; Linux is only ~40% slower, while Windows is 10x slower. On Windows, 70% of time is spent in CollectFallbackHintChars. This is being tracked in crbug.com/848295 . Bug: 636993, 645035 Change-Id: Ia1af2f0530d8a4db1c12b7e2059f5926165f6ffc Reviewed-on: https://chromium-review.googlesource.com/1083914 Commit-Queue: Koji Ishii <kojii@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Reviewed-by: Dominik Röttsches <drott@chromium.org> Cr-Commit-Position: refs/heads/master@{#564363} [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css2.1/t0905-c414-flt-fit-01-d-g-expected.txt [add] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css2.1/t0905-c5525-flthw-00-c-g-expected.txt [add] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/css2.1/t0905-c5526-flthw-00-c-g-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/editing/selection/dont-select-text-overflow-ellipsis-when-wrapping-rtl-mixed-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/editing/selection/select-text-overflow-ellipsis-mixed-in-rtl-2-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/editing/selection/select-text-overflow-ellipsis-mixed-in-rtl-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/css/rtl-ordering-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/css/text-overflow-ellipsis-bidi-expected.txt [add] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/encoding/invalid-UTF-8-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/forms/select/select-visual-hebrew-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/forms/select/select-writing-direction-natural-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/forms/visual-hebrew-text-field-expected.txt [add] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/justify-ideograph-complex-expected.png [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/justify-ideograph-simple-expected.png [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/justify-ideograph-vertical-expected.png [delete] https://crrev.com/3860f3e4d199e934052b3486a226dde44fb22e9a/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/selection/selection-multiple-runs-expected.png [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/trailing-white-space-2-expected.png [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/trailing-white-space-expected.png [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/xsl/xslt-enc-cyr-expected.txt [add] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/svg/hixie/intrinsic/003-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/transforms/2d/hindi-rotated-expected.txt [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/core/layout/ng/inline/ng_inline_item.cc [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/core/layout/ng/inline/ng_inline_item.h [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.h [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/core/layout/ng/inline/ng_inline_node_test.cc [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/platform/fonts/shaping/harf_buzz_shaper.cc [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/platform/fonts/shaping/harf_buzz_shaper.h [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/platform/fonts/shaping/shaping_line_breaker.cc [modify] https://crrev.com/06e705c947d8a481412c9a87c83b862469ef3929/third_party/blink/renderer/platform/fonts/shaping/shaping_line_breaker.h
,
Jun 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e98748af0f83c1ea9ffac5f2c95ce9cadf7a87a6 commit e98748af0f83c1ea9ffac5f2c95ce9cadf7a87a6 Author: Koji Ishii <kojii@chromium.org> Date: Mon Jun 18 18:29:59 2018 Remove CachedRunSegmenter This patch removes CachedRunSegmenter, introduced in CL:1053346, because CL:1083914 supercedes this improvement by LayoutNG not invoking RunSegmenter multiple times. Other recent changes such as pre-allocating are preserved as they are still valid. Bug: 636993, 645035 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng Change-Id: I83e74a58b79b856df1f046b7fa90b485609cd85a Reviewed-on: https://chromium-review.googlesource.com/1087191 Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#568085} [modify] https://crrev.com/e98748af0f83c1ea9ffac5f2c95ce9cadf7a87a6/third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc [modify] https://crrev.com/e98748af0f83c1ea9ffac5f2c95ce9cadf7a87a6/third_party/blink/renderer/platform/fonts/shaping/harf_buzz_shaper.cc [modify] https://crrev.com/e98748af0f83c1ea9ffac5f2c95ce9cadf7a87a6/third_party/blink/renderer/platform/fonts/shaping/harf_buzz_shaper.h [modify] https://crrev.com/e98748af0f83c1ea9ffac5f2c95ce9cadf7a87a6/third_party/blink/renderer/platform/fonts/shaping/run_segmenter.cc [modify] https://crrev.com/e98748af0f83c1ea9ffac5f2c95ce9cadf7a87a6/third_party/blink/renderer/platform/fonts/shaping/run_segmenter.h [modify] https://crrev.com/e98748af0f83c1ea9ffac5f2c95ce9cadf7a87a6/third_party/blink/renderer/platform/fonts/shaping/run_segmenter_test.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 by drott@chromium.org
, Sep 8 2016