[LayoutNG] view-source: is very slow
Reported by
superpoi...@gmail.com,
Jan 4
|
||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 6.3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36 Example URL: Any Steps to reproduce the problem: 1. Go to any website 2. Ctrl-U to view source 3. What is the expected behavior? Source is viewed immediately What went wrong? Can't see the source. Canary keeps trying to get the source of the URL Did this work before? N/A Chrome version: 73.0.3660.0 Channel: canary OS Version: 8.1 Flash Version:
,
Jan 5
,
Jan 7
,
Jan 9
Unable to reproduce the issue on reported chrome version #73.0.3660.0 and latest chrome #73.0.3665.0 on Windows 10 by following steps as per comment#0. Observed that after pressing "Ctrl+u" the source of the page viewed. Note: Sometimes the page loads after 5-10 secs. As per comment#1 change log "https://chromium.googlesource.com/chromium/src/+/e68ef11a60d0bc6b496e0159c43208237e11f004", CC'ing Ian Kilpatrick to provide further inputs on the issue. Thanks..!
,
Jan 9
Seems to not work on Wordpress sites, works on other sites. (Windows 8.1) However, Chromium doesn't have this issue. Almost immediately shows the source. Can you test this site? http://wp-themes.com/twentytwelve/
,
Jan 9
The bug occurs due to LayoutNG (enabled since r572466) as force-enabling it crashes Chromium as well. Using http://wp-themes.com/twentytwelve/ Bisected with "--enable-blink-features=LayoutNG" to 567470 (good) - 567480 (bad) https://chromium.googlesource.com/chromium/src/+log/547b8f48..65d7ba6c Suspecting r567475 = crrev.com/c/1100716 by kojii@chromium.org "[LayoutNG] Improve performance for 'break-word'" Landed in 69.0.3461.0 (disabled by default) Pausing in debugger reveals a LayoutNG-related stacktrace (attached).
,
Jan 9
This seems like a LayoutNG issue. cc/ kojii@/eae@
,
Jan 10
view-source wraps every line with a table cell, which requires MinMax size. NG doesn't have specialized code path for MinMax size as we didn't want to have two different line breakers. I'm still hesitant to have a specialized line breaker for MinMax. We have some points to mitigate the problem, e.g., view-source uses `pre-wrap` and our tab handling seems slow, but still may not be as fast as legacy without the specialized line breaker. I'll try some optimizations and see what we can get. It's too slow if that was the only cause, so we may have more.
,
Jan 10
,
Jan 10
It's actually unfortunate that view-source uses table in quirks mode, which is probably one of the slowest layout algorithm. Aside we need more efforts to make it faster, probably we should switch view-source to more modern primitives?
,
Jan 10
And while you're at it, is it possible to restore the 0-delay view-source (like Chrome 42 had) for the big pages or should I open a new issue?
,
Jan 10
#11: I don't know the motivation of the change, but since you have identified the patch (thank you about that btw!) it'd be easier if you can open a separate issue. Thank you so much for tracking this down in this detail in advance.
,
Jan 10
Did you come up with a simplified testcase, Koji?
,
Jan 10
Note that it looks like this was reported before https://chromium-review.googlesource.com/c/1394600 (Use NG to measure table cell intrinsic sizes) landed. But I guess you measured with something more recent (with that CL included)?
,
Jan 10
> Did you come up with a simplified testcase Not yet, possibly there could be multiple issues. http://wp-themes.com/twentytwelve/ is quite small page but doesn't finish in several minutes, probably we're in infinite loop? https://www.ecma-international.org/ecma-262/9.0/index.html is just slow, but this page is quite big, so these two seem different problem. > Note that it looks like this was reported before... Yeah, reproduces with today's Canary. By using NG MinMax, as noted above, NG MinMax might be slower than legacy MinMax because NG uses single line breaker.
,
Jan 10
But we cache the min/max results, don't we? SetPreferredLogicalWidthsFromNG() in ng_length_utils.cc? Or are you saying that NG's line breaker makes us slower, because it performs actual layout passes in order to measure min/max?
,
Jan 10
> But we cache the min/max results, don't we? I believe so. > are you saying that NG's line breaker makes us slower, because it performs actual layout passes in order to measure min/max? Not really verified but theoretically, NG uses NGLineBreaker without running NGInlineLayoutAlgorithm, so it's faster than actual layout but still not really cheap. Legacy has a very simple line breaker just for MinMax in LayoutBlockFlow::ComputeInlinePreferredLogicalWidths(), it's quite fast. It also can compute min and max in single pass, while NG needs a pass for each. But because we have two different line breakers, we needed to implement new features in both line breakers, and sometimes they do not match. It was one of the problems we wanted to do better, so NG uses the same line breaker, which is good, but it might be slower than legacy.
,
Jan 10
Having one line breaker sounds like the right thing to me. I understand that it might be slower, but how much slower can it possibly get? 3x in the worst case?
,
Jan 10
Filed issue 920571 for modernizing view-source.
,
Jan 10
> how much slower can it possibly get? 3x in the worst case? Good point, I was thinking, we don't have micro benchmark focused on inline MinMax. Aside how commonly it is needed/used, we should at least have blink_perf test. I'll try.
,
Jan 10
Great! We definitely want that.
,
Jan 10
Do we need the line breaker at all for max? Getting the total width from each item should suffice, right? At least that way we'd only have to run the line-breaker once (for min). As for table cells and fit size couldn't we stop the line breaker as soon as it hits a break opportunity that is less than the available size in the fit-content case? Sure, the min-size won't be correct but does that really matter for fit-content and table cells with a fixed width?
,
Jan 10
Interesting thoughts. If there's some heavy machinery in the line breaker that we could skip during min/max calculation, we should try that. Especially for max, as you mention, we don't need to care about soft break opportunities at all. What does the line breaker output? For min/max, all we need is a size. If we allocate something heavier than that, we should consider skipping it somehow. However: I don't see how we can check the available size when calculating min/max (the fit-content case that you're mentioning). Otherwise min/max values won't be cacheable (what if available size changes?).
,
Jan 10
For min-size, right, I'm thinking the same optimization. Read code a bit, it looks like table cell has a bit more complex code path than regular fit-content (go through legacy) and maybe table is a bit different from regular fit-content? I don't know how table computes cell widths very well. I can try for NG fit-content first and see if it can apply to table cell case too. For max, I guess we don't need line breaker in most cases, but need to find what the exceptions are. 'text-indent' for one, wondering if floats can affect min-size? We can probably find it out.
,
Jan 10
Apart from their own contribution, floats shouldn't affect min-size. I.e. they don't affect the min-size of the inline content. They do affect max-size, though. Figuring out the inline size of table cells is indeed tricky, but all we have to worry about in this phase, is provide the correct min/max inline size of cells, and the legacy engine will do the rest of the size calculation job. Right?
,
Jan 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de4a638c44882194243ece710e6a713ba0fe2099 commit de4a638c44882194243ece710e6a713ba0fe2099 Author: Koji Ishii <kojii@chromium.org> Date: Fri Jan 11 11:14:35 2019 Add line-layout performance test for fit-content width This patch adds two line-layout performance tests: * Both have `width: fit-content`, with its parent set to `width: 400px`. * One has `word-break: break-all`, a commonly used property for source code, bug tracker, etc. Current view-source puts each line into a table cell with `word-break: break-all`. Table cells might need additional code path, but uses the common underlying code as `fit-content`. Currently with Win x64 Canary running on Z620: * line-layout-fit-content.html ~600ms. LayoutNG is ~1500ms, ~2.5x slower. * line-layout-fit-content-break-word.html ~230ms. LayoutNG is ~1150ms, ~5x slower. Note, `break-word` slows down both engines. The amount of text is adjusted to 1/5 so that tests finish within 1-2 seconds. Bug: 919123 Change-Id: Ib8646d17625e86496f262efcafb5d4e1b5cc6136 Reviewed-on: https://chromium-review.googlesource.com/c/1405108 Reviewed-by: Emil A Eklund <eae@chromium.org> Reviewed-by: Morten Stenshorne <mstensho@chromium.org> Reviewed-by: Hayato Ito <hayato@chromium.org> Commit-Queue: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#621970} [add] https://crrev.com/de4a638c44882194243ece710e6a713ba0fe2099/third_party/blink/perf_tests/layout/line-layout-fit-content-break-word.html [add] https://crrev.com/de4a638c44882194243ece710e6a713ba0fe2099/third_party/blink/perf_tests/layout/line-layout-fit-content.html
,
Yesterday
(40 hours ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/49c021d047ee19447403569a88ccdadf2df2023d commit 49c021d047ee19447403569a88ccdadf2df2023d Author: Koji Ishii <kojii@chromium.org> Date: Mon Jan 21 14:36:55 2019 [LayoutNG] Improve performance of min-content for break-word |NGLineBreaker::HandleOverflow()| had an old code that is no longer needed. It has no effects on results, but removing it improves `line-layout-fit-content-break-word.html` by 30%. It was probably needed at some point but then became redundant. This patch removes the condition. There should be no behavior changes. blink_perf.layout_ng: https://pinpoint-dot-chromeperf.appspot.com/job/179e3a0a540000 loading.desktop_layout_ng looks noise: https://pinpoint-dot-chromeperf.appspot.com/job/14818d71540000 Probably fit-content+break-word isn't used much in top sites. Bug: 919123 Change-Id: I7e296b083144de6b2813a39d36701f039d7d5053 Reviewed-on: https://chromium-review.googlesource.com/c/1424105 Reviewed-by: Morten Stenshorne <mstensho@chromium.org> Commit-Queue: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#624581} [modify] https://crrev.com/49c021d047ee19447403569a88ccdadf2df2023d/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by woxxom@gmail.com
, Jan 47.6 MB
7.6 MB Download