Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1525963002:
(CANCELED) Improve performance of large option list with CJK text (Closed)
|
Created:
5 years ago by kojii Modified:
5 years ago CC:
chromium-reviews, krit, szager+layoutwatch_chromium.org, drott+blinkwatch_chromium.org, Rik, zoltan1, blink-reviews-platform-graphics_chromium.org, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, Justin Novosad, jbroman, danakj, pdr+graphicswatchlist_chromium.org, f(malita), jchaffraix+rendering, blink-reviews, vmpstr+blinkwatch_chromium.org, Stephen Chennney, dshwang, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove performance of large option list with CJK text
This patch adds Font::widthForUnbrokenRun() that can take advantages of
the word cache in CachingWordShaper for arbitrary length of Asian text.
CachingWordShaper delimits text by spaces, so it does not split scripts
that do not use spaces such as Asian scripts.
The primary use of Font::width() is the line breaker, and it queries
by splitting text at break opportunities. Since such Asian scripts have
break opportunity between every character, the way CachingWordShaper
handles "word" works good for the line breaker.
The <select> element measures all text in its <option> elements to
determine its width, and since each <option> is usually unique text,
this characteristic of CachingWordShaper slows it down when it has many
<option> elements.
A benchmark of 2352 option elements with CJK text was slower than ASCII
by 6-7 times. This patch improves it to 20-30% slower.
BUG=565902
Patch Set 1 : Fix LayoutMenuList only #Patch Set 2 : Typo and minor edits #Patch Set 3 : Strategy changed to fix all code paths #Patch Set 4 : Fix unsigned and add test #Patch Set 5 : Revert to PS2 #
Total comments: 1
Messages
Total messages: 18 (12 generated)
Description was changed from ========== WIP: select-many-options BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries for each break opportunity. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" is not an issue for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements for CJK text was slower than ASCII by 6 times before this patch, but it is improved to 20~30% slower. BUG=565902 ==========
Description was changed from ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries for each break opportunity. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" is not an issue for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements for CJK text was slower than ASCII by 6 times before this patch, but it is improved to 20~30% slower. BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries for each break opportunity. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" is not an issue for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6 times before this patch, but it is improved to 20~30% slower. BUG=565902 ==========
Description was changed from ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries for each break opportunity. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" is not an issue for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6 times before this patch, but it is improved to 20~30% slower. BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries for each break opportunity. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" is not an issue for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by ~7 times before this patch, but it is improved to 20~30% slower. BUG=565902 ==========
Description was changed from ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries for each break opportunity. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" is not an issue for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by ~7 times before this patch, but it is improved to 20~30% slower. BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries for each break opportunity. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" is not an issue for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times before this patch, but it is improved to 20~30% slower. BUG=565902 ==========
Description was changed from ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries for each break opportunity. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" is not an issue for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times before this patch, but it is improved to 20~30% slower. BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries by splitting text at break opportunities. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" works good for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times before this patch, but it is improved to 20~30% slower. BUG=565902 ==========
Description was changed from ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries by splitting text at break opportunities. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" works good for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times before this patch, but it is improved to 20~30% slower. BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries by splitting text at break opportunities. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" works good for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times. This patch improves it to 20~30% slower. BUG=565902 ==========
Description was changed from ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries by splitting text at break opportunities. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" works good for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times. This patch improves it to 20~30% slower. BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch changes CachingWordShapeIterator to handle one CJK character as a word for the caching purpose. Since these scripts do not delimit words by spaces, text in these scripts do not perform well with the CachingWordShaper. However, since these scripts have break opportunities between almost every character, and since the line breaker, the most performance critical user of CachingWordShaper splits text by break opportunities, this characteristic is not critical for the layout purpose. The <select> element measures the width of all text in its <option> elements to determine its width, and since each <option> usually has unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times. This patch improves it to 10~20% slower on debug builds. BUG=565902 ==========
Description was changed from ========== Improve performance of large option list with CJK text This patch changes CachingWordShapeIterator to handle one CJK character as a word for the caching purpose. Since these scripts do not delimit words by spaces, text in these scripts do not perform well with the CachingWordShaper. However, since these scripts have break opportunities between almost every character, and since the line breaker, the most performance critical user of CachingWordShaper splits text by break opportunities, this characteristic is not critical for the layout purpose. The <select> element measures the width of all text in its <option> elements to determine its width, and since each <option> usually has unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times. This patch improves it to 10~20% slower on debug builds. BUG=565902 ========== to ========== Change CachingWordShapeIterator to handle one CJK character as a word This patch changes CachingWordShapeIterator to handle one CJK character as a word for the caching purpose. Since these scripts do not delimit words by spaces, text in these scripts do not perform well with the CachingWordShaper. However, since these scripts have break opportunities between almost every character, and since the line breaker, the most performance critical user of CachingWordShaper, splits text by break opportunities, this characteristic is not critical for the layout purpose. The <select> element measures the width of all text in its <option> elements to determine its width, and since each <option> usually has unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements with CJK text. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times. This patch improves it to 10~20% slower on debug builds. BUG=565902 ==========
Description was changed from ========== Change CachingWordShapeIterator to handle one CJK character as a word This patch changes CachingWordShapeIterator to handle one CJK character as a word for the caching purpose. Since these scripts do not delimit words by spaces, text in these scripts do not perform well with the CachingWordShaper. However, since these scripts have break opportunities between almost every character, and since the line breaker, the most performance critical user of CachingWordShaper, splits text by break opportunities, this characteristic is not critical for the layout purpose. The <select> element measures the width of all text in its <option> elements to determine its width, and since each <option> usually has unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements with CJK text. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6~7 times. This patch improves it to 10~20% slower on debug builds. BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch changes CachingWordShapeIterator to handle one CJK character as a word for the caching purpose. Since these scripts do not delimit words by spaces, text in these scripts do not perform well with the CachingWordShaper. However, since these scripts have break opportunities between almost every character, and since the line breaker, the most performance critical user of CachingWordShaper splits text by break opportunities, this characteristic is not critical for the layout purpose. The <select> element measures the width of all text in its <option> elements to determine its width, and since each <option> usually has unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6-7 times. This patch improves it to 20-30% slower on local debug. BUG=565902 ==========
Patchset #5 (id:80001) has been deleted
Description was changed from ========== Improve performance of large option list with CJK text This patch changes CachingWordShapeIterator to handle one CJK character as a word for the caching purpose. Since these scripts do not delimit words by spaces, text in these scripts do not perform well with the CachingWordShaper. However, since these scripts have break opportunities between almost every character, and since the line breaker, the most performance critical user of CachingWordShaper splits text by break opportunities, this characteristic is not critical for the layout purpose. The <select> element measures the width of all text in its <option> elements to determine its width, and since each <option> usually has unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6-7 times. This patch improves it to 20-30% slower on local debug. BUG=565902 ========== to ========== Improve performance of large option list with CJK text This patch adds Font::widthForUnbrokenRun() that can take advantages of the word cache in CachingWordShaper for arbitrary length of Asian text. CachingWordShaper delimits text by spaces, so it does not split scripts that do not use spaces such as Asian scripts. The primary use of Font::width() is the line breaker, and it queries by splitting text at break opportunities. Since such Asian scripts have break opportunity between every character, the way CachingWordShaper handles "word" works good for the line breaker. The <select> element measures all text in its <option> elements to determine its width, and since each <option> is usually unique text, this characteristic of CachingWordShaper slows it down when it has many <option> elements. A benchmark of 2352 option elements with CJK text was slower than ASCII by 6-7 times. This patch improves it to 20-30% slower. BUG=565902 ==========
kojii@chromium.org changed reviewers: + drott@chromium.org, eae@chromium.org, tkent@chromium.org
This patch is very local to LayoutMenuList, a bit hacky. Not sure whether we should land this or not, but the concerns not to land this are: 1. The proper fix for CachingWordShapeIterator to work good with CJK text may take longer; see: https://codereview.chromium.org/1530833002/ https://codereview.chromium.org/1525963002/ 2. US/Euro is ooo in year end, while Asia is ooo after new year, so we don't have long review period until the next branch in Jan. 3. tkent considers bug 565902 is critical -- p1. If you agree to land this once, and take it out if it no longer necessary after CachingWordShapeIterator is improved, I'm fine to go. If you think we should wait to see how the improvement in CachingWordShapeIterator goes, I'm fine to wait. Your opinions are appreciated.
https://codereview.chromium.org/1525963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/Font.h (right): https://codereview.chromium.org/1525963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/Font.h:87: float widthForUnbrokenRun(const TextRun&) const; This method and its implementation, i.e. new splitting logic, should not be in Font.h. Rather I think we should have something in CachingWordShaper that allows as to use the m_singleCharMap of ShapeCache for the CJK case. If possible, I'd like to avoid "by character" special casing. Could we use FontDescription.locale() for the distinction of what kind of splitting we do in the CachingWordShaper?
On 2015/12/16 at 13:17:39, drott wrote: > https://codereview.chromium.org/1525963002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/fonts/Font.h (right): > > https://codereview.chromium.org/1525963002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/fonts/Font.h:87: float widthForUnbrokenRun(const TextRun&) const; > This method and its implementation, i.e. new splitting logic, should not be in Font.h. Rather I think we should have something in CachingWordShaper that allows as to use the m_singleCharMap of ShapeCache for the CJK case. > > If possible, I'd like to avoid "by character" special casing. Could we use FontDescription.locale() for the distinction of what kind of splitting we do in the CachingWordShaper? It's not special casing; logically speaking, CachingWordShaper should always split every character is it's CJK characters. It should not be done only in certain locale, and that's the direction the other CL is trying to do. Just that not sure if it's done by the next branch.
Just to make sure the intention is understood; this CL is probably not necessary if CachingWordShaper improvements for CJK is done before the next branch. Options as I think are: 1. Land this. If CachingWordShaper is done before the branch, I'll probably revert this. 2. Wait until, say, 1 week before the branch. If CachingWordShaper is likely to take longer, land this at that point. Revert in following branch when CachingWordShaper is done. 3. Just work on CachingWordShaper, and merge it if it's after the branch point. Option 1 looks reasonable to me, WDYT?
> Option 1 looks reasonable to me, WDYT? I prefer 2) and 3) - additional splitting should be done in CachingWordShaper.
On 2015/12/16 at 14:30:45, drott wrote: > > Option 1 looks reasonable to me, WDYT? > > I prefer 2) and 3) - additional splitting should be done in CachingWordShaper. Ok, thank you for your thoughts. Let's try to fix this all. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
