New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 622810 link

Starred by 8 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression: Slow word-break: break-all layout

Project Member Reported by allada@chromium.org, Jun 23 2016

Issue description

When an element has word-break set to break-all (maybe some others as well) the layout takes a long time if there are no breaks in text.

See file. Test in Chrome (stable and/or ToT). Then test in Firefox.

This is affecting devtools in mostly networking and console and largely because of large URLs.
 
slowlayout.html
10.1 KB View Download

Comment 1 by allada@chromium.org, Jun 23 2016

Cc: l...@chromium.org

Comment 2 by allada@chromium.org, Jun 23 2016

Cc: kojii@chromium.org
Labels: -Type-Bug Type-Bug-Regression
Owner: kojii@chromium.org
I did some bisecting and here's the patch that caused the problems:

https://codereview.chromium.org/1766243003

Tagging kojii@
Labels: ReleaseBlock-Stable M-52
Status: Assigned (was: Untriaged)
Summary: Regression: Slow word-break: break-all layout (was: Slow word-break: break-all layout)
Boosting priority of this, users are reporting 10 seconds layouts: http://stackoverflow.com/questions/37022408/chrome-v50-network-inspector-really-slow

Comment 4 by kojii@chromium.org, Jun 24 2016

Cc: e...@chromium.org
Labels: -Pri-2 -ReleaseBlock-Stable OS-All Pri-3
#3: devtool uses "break-word", not "break-all", it's about  issue 603398  and the fix was merged back to M52. Removing RBS label.

#0: Thank you for reporting.

For "break-all", please refer to comment #42 of  issue 591793 #c42. We're planning a large change that should fix this too, but it's going to take a bit.

If you have real URL that is suffered by this problem, could you please let us know, so that we can investigate to see if there were any good short term solutions.
Project Member

Comment 5 by sheriffbot@chromium.org, Jun 24 2016

Labels: -M-52 M-53 MovedFrom-52
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This is a massive issue for us. On my machine it takes *60 seconds* to do one Layout.

Here's a repro:
1. New tab
2. open devtools first, turn on device mode (the phone icon in the top right)
2. navigate the tab to https://m.washingtonpost.com/
3. Now in devtools go to Network panel.
4. once the page has loaded for several seconds, hit the red circle to stop listening to network activity. (Not required but makes it easier)
5. on the filter bar, make sure "Hide data urls" is not checked
6. scroll down to a data url. There are several "data:application/font-woff" about 30 requests from the top.
7. Click on one of them.

Results:
Laying out the Headers tab for this request takes well-over 30 seconds. 


Attached:
* screenshot showing the long layout
* screenshot showing our use of break-all
Screen Shot 2016-07-01 at 4.21.59 PM.png
563 KB View Download
Screen Shot 2016-07-01 at 4.11.22 PM.png
653 KB View Download
 Issue 621165  has been merged into this issue.
Project Member

Comment 8 by sheriffbot@chromium.org, Jul 3 2016

Labels: -M-53 MovedFrom-53
This issue has been moved once and is lower than Pri-1. Removing the milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by kojii@chromium.org, Jul 4 2016

 Issue 621165  has been merged into this issue.
#6: thanks for reporting, confirmed that Network tab, header view uses break-all in network/requestHeadersView.css.
Labels: -Pri-3 Pri-1
network/requestHeaderView.css:
.request-headers-view .outline-disclosure .header-value {
    display: inline;
    margin-right: 1em;
    white-space: pre-wrap;
    word-break: break-all;
    margin-top: 1px;
}

And also devices/devicesView.css uses break-all too:
.device-page-url a {
    color: #777;
    word-break: break-all;
}
WIP here
https://codereview.chromium.org/2077313002
but this figured out that the fix for  issue 603679  was not correct, so the fix will be a bit more complicated.

We need to try break-all, but if it fails and has floats, we need to push the line down and try break-all again. If no floats, we should fallback to first breakable.
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d24d249700deef0ed63bf12ebb6af3b28f8225fb

commit d24d249700deef0ed63bf12ebb6af3b28f8225fb
Author: kojii <kojii@chromium.org>
Date: Tue Jul 05 23:24:00 2016

Prevent to measure the whole word when break-all

This patch applies the same heuristic fix as break-word to break-all.

Since the switch to the complex path, r385693 fixed the performance and
correctness issue of break-word and break-all in common cases. However,
it also introduced a performance regression when a word is really long,
such as minimized JS.

This can be fixed by taking over the ShapeResult to the next lines as
Gecko does, but it will require larger overhaul of the line breaker and
need more time to bake.

As an interim fix, r387974 introduced a heuristic fix for break-word.
This patch applies the same heuristic to break-all.

This patch also revealed that the fix for  crbug.com/603679  was not
correct. MidWordBreak must be recomputed when floats pushed the line
down and available width changed. This patch includes the fix.

BUG= 622810 ,  603679 

Review-Url: https://codereview.chromium.org/2077313002
Cr-Commit-Position: refs/heads/master@{#403830}

[modify] https://crrev.com/d24d249700deef0ed63bf12ebb6af3b28f8225fb/third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h

Labels: Merge-Request-53
Status: Fixed (was: Assigned)
From comment #41 of  issue 591793 :

> This "word-break: break-all" example shows very poor loading and resizing performance:
> https://jsfiddle.net/4ztpbnm0/1/
> while "overflow-wrap: break-word" performs much better:
> https://jsfiddle.net/746g71wb/1/

Confirmed in Canary both runs in the similar performance.

Comment 15 by dimu@google.com, Jul 8 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Project Member

Comment 16 by bugdroid1@chromium.org, Jul 8 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b6b39703c17c81d31c03ed8d6b4ee9991a246d3

commit 8b6b39703c17c81d31c03ed8d6b4ee9991a246d3
Author: Koji Ishii <kojii@chromium.org>
Date: Fri Jul 08 07:08:17 2016

Prevent to measure the whole word when break-all

This patch applies the same heuristic fix as break-word to break-all.

Since the switch to the complex path, r385693 fixed the performance and
correctness issue of break-word and break-all in common cases. However,
it also introduced a performance regression when a word is really long,
such as minimized JS.

This can be fixed by taking over the ShapeResult to the next lines as
Gecko does, but it will require larger overhaul of the line breaker and
need more time to bake.

As an interim fix, r387974 introduced a heuristic fix for break-word.
This patch applies the same heuristic to break-all.

This patch also revealed that the fix for  crbug.com/603679  was not
correct. MidWordBreak must be recomputed when floats pushed the line
down and available width changed. This patch includes the fix.

BUG= 622810 ,  603679 

Review-Url: https://codereview.chromium.org/2077313002
Cr-Commit-Position: refs/heads/master@{#403830}
(cherry picked from commit d24d249700deef0ed63bf12ebb6af3b28f8225fb)

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

Cr-Commit-Position: refs/branch-heads/2785@{#55}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/8b6b39703c17c81d31c03ed8d6b4ee9991a246d3/third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h

Labels: Merge-Request-52
Note that the merge to M52 would probably require manual merge as I had some changes recently.

Comment 18 by dimu@google.com, Jul 9 2016

Labels: -Merge-Request-52 Merge-Approved-52
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 19 by bugdroid1@chromium.org, Jul 11 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/15a2ac4a185a2ab90a7381791ea10b403bcebf62

commit 15a2ac4a185a2ab90a7381791ea10b403bcebf62
Author: Koji Ishii <kojii@chromium.org>
Date: Mon Jul 11 12:49:07 2016

Prevent to measure the whole word when break-all

This patch applies the same heuristic fix as break-word to break-all.

Since the switch to the complex path, r385693 fixed the performance and
correctness issue of break-word and break-all in common cases. However,
it also introduced a performance regression when a word is really long,
such as minimized JS.

This can be fixed by taking over the ShapeResult to the next lines as
Gecko does, but it will require larger overhaul of the line breaker and
need more time to bake.

As an interim fix, r387974 introduced a heuristic fix for break-word.
This patch applies the same heuristic to break-all.

This patch also revealed that the fix for  crbug.com/603679  was not
correct. MidWordBreak must be recomputed when floats pushed the line
down and available width changed. This patch includes the fix.

BUG= 622810 ,  603679 

Review-Url: https://codereview.chromium.org/2077313002
Cr-Commit-Position: refs/heads/master@{#403830}
(cherry picked from commit d24d249700deef0ed63bf12ebb6af3b28f8225fb)

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

Cr-Commit-Position: refs/branch-heads/2743@{#607}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/15a2ac4a185a2ab90a7381791ea10b403bcebf62/third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h

Labels: TE-Verified-M53 TE-Verified-53.0.2785.14
Tested the issue on Windows 7, Mac 10.11.5, Ubuntu 14.04 using 53.0.2785.14 as per steps in comment #6.Observed that headers tab for 'data:application' requests loaded without any delay.

Please find attached screencast.

Marking it as TE-Verified.

622810.mp4
5.3 MB View Download

Comment 21 by phistuck@gmail.com, Jul 12 2016

Can you add a performance test for it so it does not regress again?
Labels: TE-Verified-M52 TE-Verified-52.0.2743.75
Tested the issue on Windows 7, Mac 10.11.5, Ubuntu 14.04 using 52.0.2743.75 as per steps in comment #6.Observed that headers tab for 'data:application' requests loaded without any delay.

Marking it as TE-Verified-M52.
Labels: TE-Verified-53.0.2785.21
Tested the issue on Windows 7, Mac 10.11.5, Ubuntu 14.04 using 53.0.2785.21 as per steps in comment #6.Observed that headers tab for 'data:application' requests loaded without any delay.


Cc: allada@chromium.org
 Issue 631982  has been merged into this issue.
 Issue 633410  has been merged into this issue.
Project Member

Comment 26 by bugdroid1@chromium.org, Aug 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b56d7b9d53d796b72659d6ea0a70044a74c67ca

commit 1b56d7b9d53d796b72659d6ea0a70044a74c67ca
Author: kojii <kojii@chromium.org>
Date: Tue Aug 30 04:25:56 2016

Add performance tests for long words/lines

This patch adds performance tests for:

1. To watch break-all/break-word performance for a very long word,
   which required several fixes in the past.

2. To investigate whitespace collapsing in a long line.

For 1, a test[1] indicates we're linear to the length of the word, but
Gecko/Edge looks consistent and faster than us.

For 2, we're slower than Gecko/Edge by the factor of 10 or so when the
line is very long. I will investigate it further.

[1] http://bl.ocks.org/stestagg/aa6db5d177b09e74825c6e4b02cb3f07

BUG= 622810 , 583711

Review-Url: https://codereview.chromium.org/2284113003
Cr-Commit-Position: refs/heads/master@{#415092}

[add] https://crrev.com/1b56d7b9d53d796b72659d6ea0a70044a74c67ca/third_party/WebKit/PerformanceTests/Layout/long-line-nowrap-collapse.html
[add] https://crrev.com/1b56d7b9d53d796b72659d6ea0a70044a74c67ca/third_party/WebKit/PerformanceTests/Layout/long-line-nowrap-spans-collapse.html
[add] https://crrev.com/1b56d7b9d53d796b72659d6ea0a70044a74c67ca/third_party/WebKit/PerformanceTests/Layout/long-line-nowrap.html
[add] https://crrev.com/1b56d7b9d53d796b72659d6ea0a70044a74c67ca/third_party/WebKit/PerformanceTests/Layout/resources/line-layout-perf-test.js
[add] https://crrev.com/1b56d7b9d53d796b72659d6ea0a70044a74c67ca/third_party/WebKit/PerformanceTests/Layout/word-break-break-all.html
[add] https://crrev.com/1b56d7b9d53d796b72659d6ea0a70044a74c67ca/third_party/WebKit/PerformanceTests/Layout/word-break-break-word.html
[add] https://crrev.com/1b56d7b9d53d796b72659d6ea0a70044a74c67ca/third_party/WebKit/PerformanceTests/Layout/word-wrap-break-word.html

Sign in to add a comment