Issue metadata
Sign in to add a comment
|
Massive layouts loading crbug.com/450612 |
||||||||||||||||||||||
Issue descriptionSteps 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.
,
Mar 3 2016
,
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.
,
Mar 4 2016
In that case, +drott and kojii in case someone is feeling adventurous ;)
,
Mar 4 2016
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
,
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?
,
Mar 4 2016
We're cutting M49 Stable candidate tomorrow, Friday @ 5:00 PM PST. Please try to resolve this asap.
,
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.
,
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.
,
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.
,
Mar 4 2016
,
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.
,
Mar 4 2016
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.
,
Mar 4 2016
Interestingly mozilla ignores word-break entirely for pre tags.
,
Mar 4 2016
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.
,
Mar 4 2016
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.
,
Mar 4 2016
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. ;)
,
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.
,
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?
,
Mar 4 2016
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.
,
Mar 4 2016
BreakingContext::handleText in BreakingContextInlineHeaders.h is the method we need to fix.
,
Mar 5 2016
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?
,
Mar 5 2016
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.
,
Mar 6 2016
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.
,
Mar 7 2016
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?
,
Mar 7 2016
> 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.
,
Mar 7 2016
> 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.
,
Mar 7 2016
> > 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.
,
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!
,
Mar 8 2016
#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.
,
Mar 8 2016
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 :).
,
Mar 22 2016
Getting the right fix here will likely take some time. We might want to reconsider your temporary fix kojii.
,
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
,
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
,
Apr 18 2016
,
Apr 19 2016
Issue 239302 has been merged into this issue.
,
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.
,
Apr 20 2016
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.
,
May 27 2016
Issue 611169 has been merged into this issue.
,
Jun 3 2016
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.
,
Jun 18 2016
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.
,
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.
,
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 |
|||||||||||||||||||||||
Comment 1 by le...@chromium.org
, Mar 3 2016Components: Blink>Layout
Labels: -OS-Mac Needs-Bisect OS-All