New issue
Advanced search Search tips

Issue 919123 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

[LayoutNG] view-source: is very slow

Reported by superpoi...@gmail.com, Jan 4

Issue description

UserAgent: 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:
 
Using https://www.ecma-international.org/ecma-262/9.0/index.html in step 1.
It loads completely in ~5 seconds here (up to 1 minute in the recent Chrome).
You can save it to a local file for convenience, it doesn't matter.

***************************************************************
*** from 0 to 5sec - previously the source was shown immediately as it was lazy-loading

Bisected to https://chromium.googlesource.com/chromium/src/+log/97d3d992..4ceae8dc
Suspecting r321074 "WebKit" https://chromium.googlesource.com/chromium/blink/+log/2f002c7..c4c009c

Specifically 13118c059bf843ba334e99aa296b01557c1cd0e8
"Remove ResourceFetcher's direct knowledge of Document, DocumentLoader, and Frame"
Landed in 43.0.2337.0

***************************************************************
*** from 5 to 12sec

Bisected to r361474 "Mark AlwaysUseComplexText as stable"
Landed in 49.0.2574.0

***************************************************************
*** from 12 to 18sec only in win-x86 builds for Chromium, not Chrome

Bisected to r406346 "Flip Win x86 builder on chromium waterfall to GN."
Landed in 54.0.2802.0

***************************************************************
*** 30% slowdown due to LayoutNG

Bisected to r572466 "[LayoutNG] Add a chrome:://flags for LayoutNG"
Enabling LayoutNG produces the slowdown: --enable-blink-features=LayoutNG
Disabling LayoutNG restores the old speed: --disable-blink-features=LayoutNG
This feature is enabled automatically via field trial.

***************************************************************

Three chrome://tracing results attached in lzma2 7zip format if needed.
Restoring the no-delay start and lazy-loading of the rest would be nice, if the speed can't be restored easily.
traces.7z
7.6 MB Download
Labels: Needs-Triage-M73
Components: -Internals>Network Blink>ViewSource
Cc: ikilpatrick@chromium.org swarnasree.mukkala@chromium.org
Labels: Triaged-ET
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..!
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/
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).
stacktrace.txt
7.4 KB View Download
Cc: kojii@chromium.org e...@chromium.org
Components: Blink>Layout
Labels: LayoutNG
Status: Available (was: Unconfirmed)
This seems like a LayoutNG issue. cc/ kojii@/eae@
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.
view-source-919123-http-wp-themes.com-twentytwelve.svg
141 KB Download
Cc: mstensho@chromium.org dgro...@chromium.org
Summary: [LayoutNG] view-source: is very slow (was: view-source: not working)
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?
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? 
#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.
Did you come up with a simplified testcase, Koji?
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)?
> 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.
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?
> 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.
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?
Filed issue 920571 for modernizing view-source.
> 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.
Great! We definitely want that.
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?

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?).
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.
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?
Project Member

Comment 26 by bugdroid1@chromium.org, 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

Project Member

Comment 27 by bugdroid1@chromium.org, 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