Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(177)

Issue 2285053002: Fix O(n!) in setLogicalWidthForTextRun (LayoutBlockFlowLine) (Closed)

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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+40 lines, -6 lines) Patch
M third_party/WebKit/PerformanceTests/Layout/long-line-nowrap-collapse.html View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/PerformanceTests/Layout/long-line-nowrap-spans-collapse.html View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp View 1 2 3 4 5 6 5 chunks +38 lines, -4 lines 0 comments Download

Messages

Total messages: 36 (30 generated)
kojii
PTAL.
4 years, 3 months ago (2016-08-31 15:01:33 UTC) #29
eae
LGTM This is great, thanks for working on this!
4 years, 3 months ago (2016-08-31 16:07:02 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2285053002/120001
4 years, 3 months ago (2016-08-31 16:12:38 UTC) #32
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 3 months ago (2016-08-31 16:18:05 UTC) #33
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/ccd49c63f329c36da86b128b30e277d72f5bc76d Cr-Commit-Position: refs/heads/master@{#415647}
4 years, 3 months ago (2016-08-31 16:19:53 UTC) #35
drott
4 years, 3 months ago (2016-09-02 10:03:38 UTC) #36
Message was sent while issue was closed.
Nice one!

Powered by Google App Engine
This is Rietveld 408576698