|
|
Created:
4 years, 3 months ago by kojii Modified:
4 years, 3 months ago Reviewers:
eae CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine)
When there are multiple BidiRun in a line (multiple inline elements,
whitespace collapsing, or RTL,) setLogicalWidthForTextRun has
O(#BidiRun * #WordMeasurement), resulting similar characteristics as
O(n!) for the line length.
This patch fixes this for LTR.
For RTL, we could improve up to log2(N) by using binary search, or
better by using fast index for LineLayoutText for multiple elements
case. It was once tried but removed due to its complexity, we can do it
if they came up as real cases.
Following performance tests are now 10-20x faster. The amount of text
for these tests were increased by 12x, matching to
long-line-nowrap.html.
PerformanceTests/Layout/long-line-nowrap-collapse.html
PerformanceTests/Layout/long-line-nowrap-spans-collapse.html
BUG=583711, 642345
Committed: https://crrev.com/ccd49c63f329c36da86b128b30e277d72f5bc76d
Cr-Commit-Position: refs/heads/master@{#415647}
Patch Set 1 #Patch Set 2 : WIP #Patch Set 3 : WIP #Patch Set 4 : WIP #Patch Set 5 : Try different, simpler approach #Patch Set 6 : Use lastIndex with fallback for RTL #Patch Set 7 : Comments and perf tests updated #
Messages
Total messages: 36 (30 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Still WIP BUG= ========== to ========== Fix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine) When there are multiple BidiRun in a line (multiple inline elements, whitespace collapsing, or RTL,) setLogicalWidthForTextRun has O(#BidiRun * #WordMeasurement), resulting similar characteristics as O(n!) for the line length. This patch fixes this for LTR. For RTL, we could improve up to log2(N) by using binary search, or better by using fast index for LineLayoutText for multiple elements case. It was once tried but removed due to its complexity, we can do it if that came up as a real case. BUG=583711, 642345 ==========
Description was changed from ========== Fix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine) When there are multiple BidiRun in a line (multiple inline elements, whitespace collapsing, or RTL,) setLogicalWidthForTextRun has O(#BidiRun * #WordMeasurement), resulting similar characteristics as O(n!) for the line length. This patch fixes this for LTR. For RTL, we could improve up to log2(N) by using binary search, or better by using fast index for LineLayoutText for multiple elements case. It was once tried but removed due to its complexity, we can do it if that came up as a real case. BUG=583711, 642345 ========== to ========== Fix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine) When there are multiple BidiRun in a line (multiple inline elements, whitespace collapsing, or RTL,) setLogicalWidthForTextRun has O(#BidiRun * #WordMeasurement), resulting similar characteristics as O(n!) for the line length. This patch fixes this for LTR. For RTL, we could improve up to log2(N) by using binary search, or better by using fast index for LineLayoutText for multiple elements case. It was once tried but removed due to its complexity, we can do it if that came up as a real case. Following performance tests are now 10-20x faster. The amount of text for these tests were increased by 12x, matching to long-line-nowrap.html. PerformanceTests/Layout/long-line-nowrap-collapse.html PerformanceTests/Layout/long-line-nowrap-spans-collapse.html BUG=583711, 642345 ==========
Description was changed from ========== Fix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine) When there are multiple BidiRun in a line (multiple inline elements, whitespace collapsing, or RTL,) setLogicalWidthForTextRun has O(#BidiRun * #WordMeasurement), resulting similar characteristics as O(n!) for the line length. This patch fixes this for LTR. For RTL, we could improve up to log2(N) by using binary search, or better by using fast index for LineLayoutText for multiple elements case. It was once tried but removed due to its complexity, we can do it if that came up as a real case. Following performance tests are now 10-20x faster. The amount of text for these tests were increased by 12x, matching to long-line-nowrap.html. PerformanceTests/Layout/long-line-nowrap-collapse.html PerformanceTests/Layout/long-line-nowrap-spans-collapse.html BUG=583711, 642345 ========== to ========== Fix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine) When there are multiple BidiRun in a line (multiple inline elements, whitespace collapsing, or RTL,) setLogicalWidthForTextRun has O(#BidiRun * #WordMeasurement), resulting similar characteristics as O(n!) for the line length. This patch fixes this for LTR. For RTL, we could improve up to log2(N) by using binary search, or better by using fast index for LineLayoutText for multiple elements case. It was once tried but removed due to its complexity, we can do it if they came up as real cases. Following performance tests are now 10-20x faster. The amount of text for these tests were increased by 12x, matching to long-line-nowrap.html. PerformanceTests/Layout/long-line-nowrap-collapse.html PerformanceTests/Layout/long-line-nowrap-spans-collapse.html BUG=583711, 642345 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kojii@chromium.org changed reviewers: + eae@chromium.org
PTAL.
LGTM This is great, thanks for working on this!
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Fix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine) When there are multiple BidiRun in a line (multiple inline elements, whitespace collapsing, or RTL,) setLogicalWidthForTextRun has O(#BidiRun * #WordMeasurement), resulting similar characteristics as O(n!) for the line length. This patch fixes this for LTR. For RTL, we could improve up to log2(N) by using binary search, or better by using fast index for LineLayoutText for multiple elements case. It was once tried but removed due to its complexity, we can do it if they came up as real cases. Following performance tests are now 10-20x faster. The amount of text for these tests were increased by 12x, matching to long-line-nowrap.html. PerformanceTests/Layout/long-line-nowrap-collapse.html PerformanceTests/Layout/long-line-nowrap-spans-collapse.html BUG=583711, 642345 ========== to ========== Fix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine) When there are multiple BidiRun in a line (multiple inline elements, whitespace collapsing, or RTL,) setLogicalWidthForTextRun has O(#BidiRun * #WordMeasurement), resulting similar characteristics as O(n!) for the line length. This patch fixes this for LTR. For RTL, we could improve up to log2(N) by using binary search, or better by using fast index for LineLayoutText for multiple elements case. It was once tried but removed due to its complexity, we can do it if they came up as real cases. Following performance tests are now 10-20x faster. The amount of text for these tests were increased by 12x, matching to long-line-nowrap.html. PerformanceTests/Layout/long-line-nowrap-collapse.html PerformanceTests/Layout/long-line-nowrap-spans-collapse.html BUG=583711, 642345 Committed: https://crrev.com/ccd49c63f329c36da86b128b30e277d72f5bc76d Cr-Commit-Position: refs/heads/master@{#415647} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ccd49c63f329c36da86b128b30e277d72f5bc76d Cr-Commit-Position: refs/heads/master@{#415647}
Message was sent while issue was closed.
Nice one! |