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

Issue 591793 link

Starred by 12 users

Issue metadata

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



Sign in to add a comment

Massive layouts loading crbug.com/450612

Project Member Reported by le...@chromium.org, Mar 3 2016

Issue description

Steps to reproduce: load  crbug.com/450612 

The tab hangs for long periods of time on Canary (51.0.2665.0) and Dev for me. Perhaps there's a regression? Does it happen on all platforms?

Tracing doesn't provide more info than just "layout," but I attached a trace.

14 seconds! The page is *not* that big.
 
trace_longlayout.json.gz
1.6 MB Download

Comment 1 by le...@chromium.org, Mar 3 2016

Cc: cbiesin...@chromium.org szager@chromium.org
Components: Blink>Layout
Labels: -OS-Mac Needs-Bisect OS-All
Adding some more folks. 

Repros in Linux. Safari and FFX have very little trouble. Let's see if there's a range.

Comment 2 by le...@chromium.org, Mar 3 2016

Labels: Performance

Comment 3 by e...@chromium.org, Mar 3 2016

Might be the lack of a max-length for shaping biting us again. We really should force segmentation after 128 or 256 characters.

Comment 4 by le...@chromium.org, Mar 4 2016

Cc: drott@chromium.org kojii@chromium.org
In that case, +drott and kojii in case someone is feeling adventurous ;)

Comment 5 by ajha@chromium.org, Mar 4 2016

Cc: -e...@chromium.org gov...@chromium.org ajha@chromium.org sshru...@google.com
Labels: -Type-Bug -Pri-2 -Needs-Bisect M-49 ReleaseBlock-Stable hasbisect Type-Bug-Regression Pri-1
Owner: e...@chromium.org
Status: Assigned (was: Available)
Able to reproduce this on the latest canary(51.0.2666.0) and the latest stable(49.0.2623.75) on Windows-7, Mac OS 10.11.3 and Linux Ubuntu 14.04.

This has the same regression range as  Issue 590436 . Marking this RB-Stable for getting a fix before scheduled stable refresh or subsequent M-49 roll-out.

Last good build: 49.0.2573.0
First bad build: 49.0.2574.0

Changelog:https://chromium.googlesource.com/chromium/src/+log/20f3b2d0ee459ed4d743ce9cd8c95417d74f6d04..fa7fc32c5940dfd3d734ed3231b1295da4c3303e

Suspect: https://codereview.chromium.org/1474673003




Comment 6 by kojii@chromium.org, Mar 4 2016

Huh, a long list of URLs is fun for the caching word shaper. I think eae@ is talking about hb being slow for a long string because it's not O(n) for the length of the input string, and max-length might help that.

Another possible issue is that we're unlikely to hit the word caching at all. When the cache doesn't hit, pages are quite slow, such as  bug 588570  or  bug 588569 .

Maybe segment more, like at "/", if the font doesn't have kerning/joining for "/" might help for URL case? I believe we already have that code for space character?
Cc: ligim...@chromium.org
We're cutting M49 Stable candidate tomorrow, Friday @ 5:00 PM PST. Please try to resolve this asap. 

Comment 8 by kojii@chromium.org, Mar 4 2016

I don't think this is a blocker/p1. The case needs to be supported better though. I'll let eae@ for the call.

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

and monorail sets word-break:brak-all and word-wrap:break-word for <a>, which makes things worse. In our current line breaking code, these properties increase calls to width() a lot more. leviw@ and I discussed on this a bit, but this is quite a work to fix.

Comment 10 by e...@chromium.org, Mar 4 2016

This is pretty slow already with a layout time of over 3.5s in 48 due to the combination of break-all/break-word and long urls. 49 made it quite a bit worse with layout times of over 15 seconds.

Adding slash segmentation doesn't do much.

Removing the break-all/break-word rules brings rendering time down to <100ms.

Comment 11 by e...@chromium.org, Mar 4 2016

Labels: -ReleaseBlock-Stable

Comment 12 by e...@chromium.org, Mar 4 2016

word-wrap:break-word isn't too bad, it's break-all that is causing the vast majority of the extra complexity.
Thanks for checking, I thought both are similarly bad, that's a good news. It's still hard to fix, we need to make line breaking context rewindable, and figuring out which state we need to keep to rewind isn't easy.

Monorail shouldn't use break-all btw, it looks quite bad when it breaks in the middle of bug number.

Comment 14 by e...@chromium.org, Mar 4 2016

Interestingly mozilla ignores word-break entirely for pre tags.
word-wrap currently measures every single character, so it actually should hit the cache quite efficiently, but it does not measure ligatures/joining correctly.

break-all, on the other hand, measures all substrings of a word; i.e., "w", "wo", "wor", and "word", so I guess cache is overflowing and being cleared.

Gecko looks like they're using word-break property of the containing block, it works if I set to <pre>, but setting to <a> doesn't work. The perf does not seem to be different when it's set or not.

Comment 16 by e...@chromium.org, Mar 4 2016

Cc: behdad@chromium.org
A couple of ideas:

1) We could check if the font has ligatures and if not accumulate the width instead of re-measuring every substring.
2) We could check if the entire text content fits on one line before doing the break opportunity loop, thereby avoiding the overhead for lines that do not require breaking.
3) Building on 2, for lines that do break we could use the measurement for the entire text content and get the character index for place where we have to break (from ShapeResult) and then subtract the max number of context charters and start from there until we find the line break and then repeat for each line.

Option 3 seems like a decent option but will be a bit complex to implement.
Option 1 or 2 might be a good enough stop-gap for now.

1) We could check if the font has ligatures and if not accumulate the width instead of re-measuring every substring.

That's definitely a good idea. Is this common? (I'm ignorant about what it's normal for fonts to have)

2) We could check if the entire text content fits on one line before doing the break opportunity loop, thereby avoiding the overhead for lines that do not require breaking.

I think word-at-a-time is the right tradeoff here. Then one can go back and start again when the word doesn't fit. Ideally, we also measure all the way to the end of a ligature when we have one. That would fix our issue with sans-bullshit-sans (crbug.com/479370).

3) Building on 2, for lines that do break we could use the measurement for the entire text content and get the character index for place where we have to break (from ShapeResult) and then subtract the max number of context charters and start from there until we find the line break and then repeat for each line.

That scares me. ;)

Comment 18 by e...@chromium.org, Mar 4 2016

The problem with word-at-a-time is that it translates to character-at-a-time for break-all. Measuring every possible substring even in cases where there is no need for breaking seems unnecessarily expensive.

We already have the max-preferred-width in many cases, comparing that to the available space and short-circuiting seems like it would be a good and safe optimization.

Comment 19 by e...@chromium.org, Mar 4 2016

Drott, Behdad, do you have any ideas? The problem is that for the string "hello world" we shape and measure the following:

"h"
"he"
"hel"
"hell"
"hello"
"hello "
"hello w"
"hello wo"
"hello wor"
"hello worl"
"hello world"

Perhaps we could keep the harfbuzz buffer around from the first one and append to it to re-shape?
Ouch!  We should definitely try to find the approximate line break location by just walking the shape results and only reshape the two parts of whatever word we end up splitting.

Just binary searching instead of linear searching in what we're doing right now might also be an acceptable solution in the interim.

Comment 21 by e...@chromium.org, Mar 4 2016

BreakingContext::handleText in BreakingContextInlineHeaders.h is the method we need to fix.
There's a spec-level question that if a word breaks because of break-all, how do we shape it.

The current code went one way that each broken component should be shaped, and to do that, we need to measure as we do today, in comment #19.

Fortunately or not, the current CSS Text 3 went another way saying:
> When shaping scripts such as Arabic are allowed to break within words due to break-all,
> the characters must still be shaped as if the word were not broken.

so this allows us to measure a word or entire text, then split by accumulating advances. This was put in a few years ago by a request from mozilla, so they might have thought about this issue.

Hard part of this method is that, after we broke, we need to render part of a word in a joined form, and we don't support that today.

We have some open issues around measuring joining scripts and ligatures when break-all/break-word, so my take is to fix the measuring as CSS Text 3 says, and the rendering fix can be deferred in future. This should mitigate this perf issue. Does that sound reasonable?
We're collapsing spaces during this process in handleText(), so measuring the entire text need to duplicate collapsing space code.

"Go with normal word breaking with step back at the line end" still looks the reasonable approach. Could be easy, could be complex, I need to try it out to understand how easy or hard it is.

It's possible that there's cheaper approach. If it works, it's probably smaller change and easier to merge. It may not work well for joining/ligatures, but it should be still the same level as break-word.

I'll see how the 2 approaches would look like.
word-break:break-word (not word-wrap:break-word) us as slow as break-all, so we probably need to fix preferred width for LayoutText.
Behdad, if I understood your plans correctly, isn't this another case of the "safe-to-break" info in the shaping result? If we had that, couldn't we just slide along the shape result and use those "safe-to-break" boundaries for a slightly-bent break-all, where we avoiding splitting ligatures and clusters?

> only reshape the two parts of whatever word we end up splitting.

The "only reshape" is not necessary, as the spec says:
> Shaping characters are still shaped as if the word were not broken
https://drafts.csswg.org/css-text-3/#valdef-overflow-wrap-break-word
https://drafts.csswg.org/css-text-3/#valdef-word-break-keep-all

But then the challenge would be, can we render a broken word still shaped as if it's not broken?

I understand how to measure as the spec defines, but I'm not sure how we could render that way.
> Behdad, if I understood your plans correctly, isn't this another case of the "safe-to-break" info in the shaping result? If we had that, couldn't we just slide along the shape result and use those "safe-to-break" boundaries for a slightly-bent break-all, where we avoiding splitting ligatures and clusters?

Correct!  That's the way it should be done IMO indeed.
> > only reshape the two parts of whatever word we end up splitting.
> 
> The "only reshape" is not necessary, as the spec says:
> > Shaping characters are still shaped as if the word were not broken
> https://drafts.csswg.org/css-text-3/#valdef-overflow-wrap-break-word
> https://drafts.csswg.org/css-text-3/#valdef-word-break-keep-all

Bad bad quoting!   That sentence in full reads: "When shaping scripts such as Arabic are allowed to break within words due to break-all, the characters must still be shaped as if the word were not broken."

What it means is that Arabic-style joining should happen.  But you must reshape to get correct results.  Otherwise, what do you do if you breaking in the middle of a big ligature?!

The Arabic joining will happen correctly because in Blink we pass the entire text to HarfBuzz, and HarfBuzz knows how to shape correctly in context.

Comment 29 by e...@chromium.org, Mar 7 2016

I have some ideas about how to do this based on shaping information and will do some experimentation this week to see if it can be made to work. I'll keep this bug up to date. If anyone wants to help please talk to me offline!
#28:
> What it means is that Arabic-style joining should happen.
> But you must reshape to get correct results.
Hmm, I still read the spec differently (i.e., not to reshape.) We can ask for clarification, and/or if reshaping is better, we could add clarification to the spec. Either way, if two people read the text differently, it should better clarify. It'll go to www-style, though the person who made the original proposal has left, we may or may not get quick response.

> Otherwise, what do you do if you breaking in the middle of a big ligature?!
It says "at arbitrary", so you don't have to break single glyph.

I replied to your ask for clarification on the www-style list.  You are quoting me wrong there :).

Try it with hb-shape yourself.  Get any Arabic font, shape this letter:

$ hb-shape NotoNaskhArabic-Regular.ttf --text=ب
[uni0628=0+1581]

Now, provide same letter as pre-context:

$ hb-shape NotoNaskhArabic-Regular.ttf --text=ب --text-before=ب
[uni0628.fina=0+1673]

Or post-context:

$ hb-shape NotoNaskhArabic-Regular.ttf --text=ب --text-after=ب
[uni0628.init=0+564]

Or both:

$ hb-shape NotoNaskhArabic-Regular.ttf --text=ب --text-before=ب --text-after=ب
[uni0628.medi=0+599]

That's what it means when CSS says "as if it was not broken".  BTW, CSS is not the authority on what's typographically right.  It tries to document it, but is no replacement :).

Comment 32 by e...@chromium.org, Mar 22 2016

Labels: -Pri-1 Pri-2
Getting the right fix here will likely take some time. We might want to reconsider your temporary fix kojii.
Project Member

Comment 33 by bugdroid1@chromium.org, Apr 7 2016

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

commit d12b58ef0005d24dff2047f566f5ff5a5f2e45fb
Author: kojii <kojii@chromium.org>
Date: Thu Apr 07 08:27:41 2016

Improve word-break: break-all and word-wrap: break-word

This patch improves:
1. Performance of "word-wrap: break-word" and "word-break: break-all".
2. "word-wrap: break-word" no longer breaks shaping scripts when it
   should not ( crbug.com/380667 ).

This patch does not give a complete support when a shaped word is
broken by these properties. More accurate break using the safe-to-break
feature in HarfBuzz and rendering support will be in future patches.

Before this patch, BreakingContext::handleText() measures all
substrings to measure the correct width of joining scripts. For a
"word", this means it measures "w", "wo", "wor", and "word". This
method does not work when a shaped words can be shorter by adding more
characters.

With this patch, Blink tries normal line breaking. Then at the break
point, Blink rewinds the break point to the largest number of glyphs
that can fit.

BUG= 380667 ,  591793 , 479370

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

Cr-Commit-Position: refs/heads/master@{#385693}

[modify] https://crrev.com/d12b58ef0005d24dff2047f566f5ff5a5f2e45fb/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/d12b58ef0005d24dff2047f566f5ff5a5f2e45fb/third_party/WebKit/LayoutTests/fast/text/break-word-fit-content-arabic-expected.html
[add] https://crrev.com/d12b58ef0005d24dff2047f566f5ff5a5f2e45fb/third_party/WebKit/LayoutTests/fast/text/break-word-fit-content-arabic.html
[add] https://crrev.com/d12b58ef0005d24dff2047f566f5ff5a5f2e45fb/third_party/WebKit/LayoutTests/fast/text/break-word-pre-wrap-expected.html
[add] https://crrev.com/d12b58ef0005d24dff2047f566f5ff5a5f2e45fb/third_party/WebKit/LayoutTests/fast/text/break-word-pre-wrap.html
[modify] https://crrev.com/d12b58ef0005d24dff2047f566f5ff5a5f2e45fb/third_party/WebKit/Source/core/layout/line/BreakingContextInlineHeaders.h

Project Member

Comment 34 by bugdroid1@chromium.org, Apr 7 2016

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

commit 4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8
Author: Rebaseline Bot <blink-rebaseline-bot@chromium.org>
Date: Thu Apr 07 10:19:13 2016

Auto-rebaseline for r385693

https://chromium.googlesource.com/chromium/src/+/d12b58ef0

BUG= 591793 
TBR=kojii@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#385704}

[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/basic-textareas-expected.png
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/basic-textareas-expected.txt
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/basic-textareas-quirks-expected.png
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/forms/textarea/basic-textareas-quirks-expected.txt
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/text/midword-break-after-breakable-char-expected.png
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/forms/textarea/basic-textareas-expected.png
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/forms/textarea/basic-textareas-expected.txt
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/forms/textarea/basic-textareas-quirks-expected.txt
[add] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/text/midword-break-after-breakable-char-expected.txt
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac/fast/forms/textarea/basic-textareas-expected.txt
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac/fast/forms/textarea/basic-textareas-quirks-expected.txt
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac/fast/loader/text-document-wrapping-expected.txt
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac/fast/text/midword-break-after-breakable-char-expected.png
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/mac/fast/text/midword-break-after-breakable-char-expected.txt
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/win/fast/text/midword-break-after-breakable-char-expected.png
[modify] https://crrev.com/4f532a008b1a2e42c26aa8c7e5cfd72902cd03a8/third_party/WebKit/LayoutTests/platform/win/fast/text/midword-break-after-breakable-char-expected.txt

Comment 35 by e...@chromium.org, Apr 18 2016

Cc: e...@chromium.org
 Issue 603150  has been merged into this issue.

Comment 36 by e...@chromium.org, Apr 19 2016

 Issue 239302  has been merged into this issue.

Comment 37 by kojii@chromium.org, Apr 20 2016

This is now fixed in M51 (unless someone thinks it's still not enough) but caused a regression for break-word in  bug 603398 , which will be in tomorrow's Canary. I'll double-check tomorrow.

Comment 38 by kojii@chromium.org, Apr 20 2016

Owner: kojii@chromium.org
Status: Fixed (was: Assigned)
Today's Canary contains the fix for  bug 603398 .

When I load  http://crbug.com/450612  (from #0)

Stable 49.0.2623.112: Rendering 98,287ms
Canary 52.0.2712.0: Rendering 3,233ms

I call it fixed.

Comment 39 by e...@chromium.org, May 27 2016

 Issue 611169  has been merged into this issue.
I just want to check my understanding:
Since 611169 was merged with this and this is resolved, is my issue considered resolved?  I tested in Canary 53.0.2757.0 and the load speed does not appear to be improved.
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/

Why is there such an extreme performance difference? Tested both 52 and 53.

Comment 42 by kojii@chromium.org, Jun 19 2016

#40: I commented on  issue 611169  but is there a URL I can see?

#41: It's true that currently we have different performance on "word-break: break-all" and "word-wrap: break-word". Do you have a real URL suffered from "break-all" performance?

We're currently running two efforts in parallel; one is longer term but should solve performance and accuracy problems thoroughly in much better way. The other is, since that work needs more time to bake, short-term fixes for reported cases.

For the latter, there's one optimization we applied only to "break-word" because it relies on heuristics, there's small chance to break incorrectly, and "break-all" showed good enough performance for all the reported cases, so the risk of taking the heuristic didn't look worth to me at the moment.

If you have real URLs suffered from "break-all" performance, I'd be happy to look into. Your support is appreciated.

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

#41: we have a separate  issue 622810  for break-all, appreciate if you could information there.

Sign in to add a comment