New issue
Advanced search Search tips

Issue 768325 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Task

Blocking:
issue 723233



Sign in to add a comment

Loading Dispatcher v0 followup changes

Project Member Reported by toyoshim@chromium.org, Sep 25 2017

Issue description

I'm planning to make some minor changes after looking at study results.
This is a bug to track that efforts.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 26 2017

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

commit d523c9117dd36ed71304bb20844370a9f3668878
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Tue Sep 26 05:40:36 2017

ResourceLoadScheduler: switch to use DCHECK instead of SECURITY_CHECK

Since I ensure that the assumption is correct, I will replace
one existing SECURITY_CHECK with DCHECK as I planned.

Bug:  768325 
Change-Id: I1a653d8d3d3a7e62df537e9e4d3017798078a108
Reviewed-on: https://chromium-review.googlesource.com/681394
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504294}
[modify] https://crrev.com/d523c9117dd36ed71304bb20844370a9f3668878/third_party/WebKit/Source/platform/loader/fetch/ResourceLoadScheduler.cpp

Comment 2 by ricea@chromium.org, Sep 28 2017

Components: Blink>Loader
Chatted offline a bit, if we're going to separate parameters for main- and sub-frames (there's an ongoing patch) we might want to distinguish following two cases:

- 'throttled subframes that are in throttled parent frame (e.g. subframes in a background tab)', and
- 'throttled subframes that are in non-throttled parent frame (e.g. offscreen subframes in a foreground tab)

Noting these here before I forget. (Toyoshima-san's looking into the latter case a bit further- thanks!)
Study results: https://docs.google.com/document/d/1hQVIxqey7Mt3hmmKSDYr1RUOpeGbYGdhKxwj_pxoHNA/edit?usp=sharing

One of a big concern was AMP-like pages would be throttled, and as far as I investigated, AMP is happy with our throttling.


In terms of classifying iframe types, "cross origin or not" would be another considerable property. But when we want to check the parent frame's throttling state via FetchContext with a modification that allow us to track parent's FetchContext, cross origin iframe would be a little hard to track parent's state since the parent frame would be beyond a RemoteFrame. But since what we want to help is same-origin iframes, it won't be a problem. We just stop tracking parent state, and just follow the frame's throttling state.

For same-origin iframes those parent is not throttled, but the iframes are throttled, I think it would be better that blink scheduler makes throttling decisions rather than ResourceLoadScheduler does in terms of the original design policy. Otherwise, we would see inefficient mode such as CPU throttling is ON, but loading throttling is OFF for a frame.

Having different limits for the main frame and other sub-frames are independent from such throttling decision scheme. So, I will add separate limits for sub-frames.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 3 2017

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

commit f2f17adb5ac42411697665b8ca2fa2c4e3a75818
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Tue Oct 03 09:38:30 2017

ResourceLoadScheduler: add a sub-frame specific param to limit requests

The first results from beta channel trial reveals typical outstanding
request numbers of sub-frames are very different from one of the main
frame. It suggests us to have different limits for each, e.g. more
aggressive limit for sub-frames.

This patch adds a separate trial parameter to set the outstanding
limit for sub-frames so that we can study performance difference from
field trials of having different limits for the main frame and others.

Bug:  768325 
Change-Id: I82e8169a00acfa71a8895869621f7c1333902a85
Reviewed-on: https://chromium-review.googlesource.com/681098
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505993}
[modify] https://crrev.com/f2f17adb5ac42411697665b8ca2fa2c4e3a75818/testing/variations/fieldtrial_testing_config.json
[modify] https://crrev.com/f2f17adb5ac42411697665b8ca2fa2c4e3a75818/third_party/WebKit/Source/platform/loader/fetch/ResourceLoadScheduler.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 19 2017

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

commit 1318e82b383895952aef5bc8923cf9880d9b0c47
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Tue Dec 19 06:09:03 2017

ResourceLoadScheduler: remove metrics for PartiallyThrottled state

Since we haven't been interested in metrics for PartiallyThrottled
state in analyzing field-trial results, this patch removes existing
metrics for PartiallyThrottled state.

This also solves the issue that other metrics mistakenly have
variations that have the PartiallyThrottled-suffix.

Also, this patch adds an entry for ThrottlingStateChangeCount that
was missed in the histograms.xml.

Bug:  768325 
Change-Id: Ic4aaf056325897440895b9d56905d814c0cad6ff
Reviewed-on: https://chromium-review.googlesource.com/823883
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524951}
[modify] https://crrev.com/1318e82b383895952aef5bc8923cf9880d9b0c47/third_party/WebKit/Source/platform/loader/fetch/ResourceLoadScheduler.cpp
[modify] https://crrev.com/1318e82b383895952aef5bc8923cf9880d9b0c47/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)

Sign in to add a comment