[DeferUnusePreload] Preload requests via PostTask if likely unused [chromium/src : main]
Shunya Shishido (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
2 comments
Vector<KURL> potentially_unused_preloads_;How is the number of unused preloads? Will it be less than 100?
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Shunya Shishido
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
// If the preload request which is labelled as potentially unused, do not
// start the loading immediately, put the task instead.
freezable_task_runner_->PostTask(
FROM_HERE,
WTF::BindOnce(&ResourceFetcher::StartLoadAndFinishIfFailed,
WrapWeakPersistent(this), WrapPersistent(resource),
std::move(params.MutableResourceRequest().MutableBody()),
load_blocking_policy, params.GetRenderBlockingBehavior(),
params.Url()));In my understanding, `ResourceFetcher::RequestResource()` is called from the following functions via `GetImageLoader().UpdateFromElement()`. I guess these direct fetch requests can be delayed. And perhaps this might not be your intention.
- HTMLImageElement::ParseAttribute()
- HTMLImageElement::ForceReload()
- HTMLImageElement::SelectSourceURL()
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Shunya Shishido
Shunya Shishido (Gerrit)
Shunya Shishido added 2 comments![]( "Open in Gerrit")
Vector<KURL> potentially_unused_preloads_;How is the number of unused preloads? Will it be less than 100?
The number of records from the database is limited up to 10.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/predictors/loading_predictor_config.cc;l=63;drc=c3a7009be69ddc4492e8bb4e6dcb12bb0b1d3b71;bpv=1;bpt=1
Actually unused preloads are about 1.7 resources per page from UMA. I believe this is enough and using vector makes sense.
// If the preload request which is labelled as potentially unused, do not
// start the loading immediately, put the task instead.
freezable_task_runner_->PostTask(
FROM_HERE,
WTF::BindOnce(&ResourceFetcher::StartLoadAndFinishIfFailed,
WrapWeakPersistent(this), WrapPersistent(resource),
std::move(params.MutableResourceRequest().MutableBody()),
load_blocking_policy, params.GetRenderBlockingBehavior(),
params.Url()));In my understanding, `ResourceFetcher::RequestResource()` is called from the following functions via `GetImageLoader().UpdateFromElement()`. I guess these direct fetch requests can be delayed. And perhaps this might not be your intention.
- HTMLImageElement::ParseAttribute()
- HTMLImageElement::ForceReload()
- HTMLImageElement::SelectSourceURL()
As we discussed offline, `params.IsPotentiallyUnusedPreload()` in L1357 is true only when `SetIsPotentiallyUnusedPreload()` is called. `IsPotentiallyUnusedPreload()` return true when the flag is passed from the preload scanner, so this doesn't introduce effects to other callers.
Also, I removed `IsPotentiallyUnusedPreload()` check from `ResourceNeedsLoad()` in the latest patchset.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 4 comments![]( "Open in Gerrit")
Vector<KURL> potentially_unused_preloads_;Shunya ShishidoHow is the number of unused preloads? Will it be less than 100?
The number of records from the database is limited up to 10.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/predictors/loading_predictor_config.cc;l=63;drc=c3a7009be69ddc4492e8bb4e6dcb12bb0b1d3b71;bpv=1;bpt=1Actually unused preloads are about 1.7 resources per page from UMA. I believe this is enough and using vector makes sense.
Probably it's helpful to write this information in the source code comment.
locators, skip_preload_scan, potentially_unused_preloads);nit: std::move()
bool is_potentially_unused_preload_ = false;This flag is set but not used in this CL.
Do you have a plan to use this flag in subsequent CLs?
I was writing the comment below but then I noticed this flag is not used, and my comment depends on how this flag is used.
---
Currently `is_potentially_unused_preload_` means "The Resource have preloading request(s) with potentially_unused_preload LCPP flag", but maybe it should be "The Resource has only preloading request(s) with potentially_unused_preload LCPP flag and no other requests"?
(i.e. currently is_potentially_unused_preload_ remains true if the Resource is created by preloading with potentially_unused_preload LCPP flag, even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)
if (ResourceNeedsLoad(resource, policy, should_defer)) {How about reverting ShouldDeferResource() changes and:
```
if (ResourceNeedsLoad(resource, policy, should_defer)) {
if (params.IsPotentiallyUnusedPreload()) {
...PostTask...
} else {
StartLoadAndFinishIfFailed();
}
}
```
- Current code starts (async) start load even if ResourceNeedsLoad() returns false for non-IsPotentiallyUnusedPreload reasons (e.g. MHTML, already loaded resources, etc.).
- The cases where ShouldDeferResource() == true are different from what we do for IsPotentiallyUnusedPreload().
Related details
- Kouhei Ueno
- Minoru Chikamune
- Shunya Shishido
- Yoshisato Yanagisawa
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
if (ResourceNeedsLoad(resource, policy, should_defer)) {How about reverting ShouldDeferResource() changes and:
```
if (ResourceNeedsLoad(resource, policy, should_defer)) {
if (params.IsPotentiallyUnusedPreload()) {
...PostTask...
} else {
StartLoadAndFinishIfFailed();
}
}
```
- Current code starts (async) start load even if ResourceNeedsLoad() returns false for non-IsPotentiallyUnusedPreload reasons (e.g. MHTML, already loaded resources, etc.).
- The cases where ShouldDeferResource() == true are different from what we do for IsPotentiallyUnusedPreload().
- Hiroshige Hayashizaki
- Kouhei Ueno
- Shunya Shishido
- Yoshisato Yanagisawa
Shunya Shishido (Gerrit)
Shunya Shishido added 2 comments![]( "Open in Gerrit")
bool is_potentially_unused_preload_ = false;This flag is set but not used in this CL.
Do you have a plan to use this flag in subsequent CLs?I was writing the comment below but then I noticed this flag is not used, and my comment depends on how this flag is used.
---
Currently `is_potentially_unused_preload_` means "The Resource have preloading request(s) with potentially_unused_preload LCPP flag", but maybe it should be "The Resource has only preloading request(s) with potentially_unused_preload LCPP flag and no other requests"?
(i.e. currently is_potentially_unused_preload_ remains true if the Resource is created by preloading with potentially_unused_preload LCPP flag, even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)
Until the patchset 3 this flag is used, forgot to remove it in the patchset 4.
However, I slightly feel that this flag is still needed.
Let me digest your comment and update the CL again.
if (ResourceNeedsLoad(resource, policy, should_defer)) {Minoru ChikamuneHow about reverting ShouldDeferResource() changes and:
```
if (ResourceNeedsLoad(resource, policy, should_defer)) {
if (params.IsPotentiallyUnusedPreload()) {
...PostTask...
} else {
StartLoadAndFinishIfFailed();
}
}
```
- Current code starts (async) start load even if ResourceNeedsLoad() returns false for non-IsPotentiallyUnusedPreload reasons (e.g. MHTML, already loaded resources, etc.).
- The cases where ShouldDeferResource() == true are different from what we do for IsPotentiallyUnusedPreload().
Maybe you meant "reverting ResourceNeedsLoad()"?
I believe hiroshige@ meant reverting ShouldDeferResource().
Let me rewrite this.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Shunya Shishido (Gerrit)
Shunya Shishido added 4 comments![]( "Open in Gerrit")
Vector<KURL> potentially_unused_preloads_;Shunya ShishidoHow is the number of unused preloads? Will it be less than 100?
Hiroshige HayashizakiThe number of records from the database is limited up to 10.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/predictors/loading_predictor_config.cc;l=63;drc=c3a7009be69ddc4492e8bb4e6dcb12bb0b1d3b71;bpv=1;bpt=1Actually unused preloads are about 1.7 resources per page from UMA. I believe this is enough and using vector makes sense.
Probably it's helpful to write this information in the source code comment.
Done
locators, skip_preload_scan, potentially_unused_preloads);Shunya Shishidonit: std::move()
Done
bool is_potentially_unused_preload_ = false;Shunya ShishidoThis flag is set but not used in this CL.
Do you have a plan to use this flag in subsequent CLs?I was writing the comment below but then I noticed this flag is not used, and my comment depends on how this flag is used.
---
Currently `is_potentially_unused_preload_` means "The Resource have preloading request(s) with potentially_unused_preload LCPP flag", but maybe it should be "The Resource has only preloading request(s) with potentially_unused_preload LCPP flag and no other requests"?
(i.e. currently is_potentially_unused_preload_ remains true if the Resource is created by preloading with potentially_unused_preload LCPP flag, even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)
Until the patchset 3 this flag is used, forgot to remove it in the patchset 4.
However, I slightly feel that this flag is still needed.Let me digest your comment and update the CL again.
Let me keep `is_potentially_unused_preload_`.
even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)
Updated to get `is_potentially_unused_preload_` false in `Resource::MatchPreload()`, which is called if the subsequent request is non-preloading. Regarding an explicit <link rel=preload>, that's what we'd like to make some delay, so WAI. Added more tests to make the behavior explicit.
if (ResourceNeedsLoad(resource, policy, should_defer)) {Minoru ChikamuneHow about reverting ShouldDeferResource() changes and:
```
if (ResourceNeedsLoad(resource, policy, should_defer)) {
if (params.IsPotentiallyUnusedPreload()) {
...PostTask...
} else {
StartLoadAndFinishIfFailed();
}
}
```
- Current code starts (async) start load even if ResourceNeedsLoad() returns false for non-IsPotentiallyUnusedPreload reasons (e.g. MHTML, already loaded resources, etc.).
- The cases where ShouldDeferResource() == true are different from what we do for IsPotentiallyUnusedPreload().
Shunya ShishidoMaybe you meant "reverting ResourceNeedsLoad()"?
I believe hiroshige@ meant reverting ShouldDeferResource().
Let me rewrite this.
Done
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa added 2 comments![]( "Open in Gerrit")
} else if (!resource->IsPotentiallyUnusedPreload()) {I feel this part is not trivial. Questions I have had are:
1. why you need to check both params.IsPotentiallyUnusedPreload() and resource->IsPotentiallyUnusedPreload() here and there. What the difference between them? Moreover, why you need two inputs?
2. why only resource->IsPotentiallyUnusedPreload() result is respected?
3. why is it fine not to load `!resource->IsPotentiallyUnusedPreload()` case?
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Shunya Shishido
Shunya Shishido (Gerrit)
Shunya Shishido added 4 comments![]( "Open in Gerrit")
bool is_potentially_unused_preload_ = false;Shunya ShishidoThis flag is set but not used in this CL.
Do you have a plan to use this flag in subsequent CLs?I was writing the comment below but then I noticed this flag is not used, and my comment depends on how this flag is used.
---
Currently `is_potentially_unused_preload_` means "The Resource have preloading request(s) with potentially_unused_preload LCPP flag", but maybe it should be "The Resource has only preloading request(s) with potentially_unused_preload LCPP flag and no other requests"?
(i.e. currently is_potentially_unused_preload_ remains true if the Resource is created by preloading with potentially_unused_preload LCPP flag, even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)
Shunya ShishidoUntil the patchset 3 this flag is used, forgot to remove it in the patchset 4.
However, I slightly feel that this flag is still needed.Let me digest your comment and update the CL again.
Let me keep `is_potentially_unused_preload_`.
even if the Resource is actually claimed by subsequent non-preloading request or we have an explicit <link rel=preload>)
Updated to get `is_potentially_unused_preload_` false in `Resource::MatchPreload()`, which is called if the subsequent request is non-preloading. Regarding an explicit <link rel=preload>, that's what we'd like to make some delay, so WAI. Added more tests to make the behavior explicit.
As we discussed, we don't need `is_potentially_unused_preload_` in Resource anymore, and use the potentially unused preload list from the LCPP through FetchContext.
ShouldDeferResource(resource_type, params, is_potentially_unused_preload);I ended up to pass the `is_potentially_unused_preload` info outside of `ShouldDeferResource()`, because this flag is needed to schedule the loading task later in this method.
} else if (!resource->IsPotentiallyUnusedPreload()) {I feel this part is not trivial. Questions I have had are:
1. why you need to check both params.IsPotentiallyUnusedPreload() and resource->IsPotentiallyUnusedPreload() here and there. What the difference between them? Moreover, why you need two inputs?
2. why only resource->IsPotentiallyUnusedPreload() result is respected?
3. why is it fine not to load `!resource->IsPotentiallyUnusedPreload()` case?
I think comment became stale in the latest change, but let me try to reply:
`params` is the fetch parameter, which is the params for the current resource fetch. On the other hand, `resource` may come from the previous resource fetch, for example preload, memory cache etc. I used to use them to distinguish whether the current resource fetch is for the preload request or subsequent request which use previous preloaded resource.
`resource->IsPotentiallyUnusedPreload()` used to indicate "the resource is already created by the preload effort, but the actual load is scheduled, but not started yet". In that case the subsequent request doesn't start the fetch.
In the latest patch, subsequent requests start loading, and scheduled loading won't be dispatched (it's blocked in L2455).
TEST_F(ResourceFetcherTest, IsPotentiallyUnusedPreload) {I added unit tests instead of web_tests, because handling PostTask is much easier.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 1 comment![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Commit-Queue | +1 |
1 comment
!preconnected_origins_.empty() || !unused_preloads_.empty();This is not directly related to this CL, but maybe I should add it.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
!preconnected_origins_.empty() || !unused_preloads_.empty();This is not directly related to this CL, but maybe I should add it.
This is not directly related to this CL, but maybe I should add it.
I think you shouldn't update this. This is only used by the following place to record `Blink.LCPP.PotentiallyLCPResourcePriorityBoosts2`. Your experiment doesn't boost fetch priority.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
!preconnected_origins_.empty() || !unused_preloads_.empty();Minoru ChikamuneThis is not directly related to this CL, but maybe I should add it.
This is not directly related to this CL, but maybe I should add it.
I think you shouldn't update this. This is only used by the following place to record `Blink.LCPP.PotentiallyLCPResourcePriorityBoosts2`. Your experiment doesn't boost fetch priority.
OK removed. But that sounds the method name is confusing then...
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno added 1 comment![]( "Open in Gerrit")
// predcitor, which is attached to the frame. This returns nullptr for Sharedempty Vector
Related details
- Hiroshige Hayashizaki
- Minoru Chikamune
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
// predcitor, which is attached to the frame. This returns nullptr for SharedShunya Shishidoempty Vector
Done
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
Perhaps kLCPPDeferUnusedPreload flag is not checked in the read path? If so, please add it to disable this feature in Control group.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
Perhaps kLCPPDeferUnusedPreload flag is not checked in the read path? If so, please add it to disable this feature in Control group.
I believe we have it.
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/predictors/lcp_critical_path_predictor/lcp_critical_path_predictor_util.cc;l=693
Let me know if this check isn't enough.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
// If |resource| is potentially unused preload based on the LCPP hint,
// schedule the loading instead of calling `StartLoad()`.
if (is_potentially_unused_preload) {
ScheduleLoadingPotentiallyUnusedPreload(resource);
}Note to self: I'm not sure this part is OK or not.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
// If |resource| is potentially unused preload based on the LCPP hint,
// schedule the loading instead of calling `StartLoad()`.
if (is_potentially_unused_preload) {
ScheduleLoadingPotentiallyUnusedPreload(resource);
}Note to self: I'm not sure this part is OK or not.
`is_potentially_unused_preload` will be true only when the resource load will be delayed, and `should_defer` will return true as well. Therefore `ResourceNeedsLoad()` will be false and the resource load is not started. We schedule resource load instead.
I believe it was fine to schedule it in a single `if (is_potentially_unused_preload)` condition. But updated to schedule inside `else if (is_potentially_unused_preload && should_defer)` for the clarity, which guarantees the resource load is not started yet.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Minoru Chikamune
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 3 comments![]( "Open in Gerrit")
Vector<KURL> FrameFetchContext::GetPotentiallyUnusedPreloads() const {returning `const Vector<KURL>&`
or defining `FetchContext::IsPotentiallyUnusedPreloadURL(const KURL& url)`
is better to avoid a copy.
ScheduleLoadingPotentiallyUnusedPreload(resource);This still schedule a load for resources that should be deferred due to non-potentially_unused_preload reasons.
(e.g. preload scanner's request for <img> in an image-disabled Document -- `should_defer` is false for the kImage-related condition, but this section triggers a load due to is_potentially_unused_preload)
Perhaps ShouldDeferResource should return a three-state enum value (no-defer/defer/defer-and-schedule).
resource->FinishAsError(ResourceError::CancelledError(resource->Url()),nit: the existing callers for `ResourceFetcher::StartLoad(Resource*)` don't call FinishAsError() on error.
So this looks a little inconsistent (with those existing callers), but it's probably OK -- anyway this is consistent with Line 1377.
Related details
- Kouhei Ueno
- Minoru Chikamune
- Shunya Shishido
- Yoshisato Yanagisawa
Minoru Chikamune (Gerrit)
Minoru Chikamune added 1 comment![]( "Open in Gerrit")
// If |resource| is potentially unused preload based on the LCPP hint,
// schedule the loading instead of calling `StartLoad()`.
if (is_potentially_unused_preload) {
ScheduleLoadingPotentiallyUnusedPreload(resource);
}Shunya ShishidoNote to self: I'm not sure this part is OK or not.
`is_potentially_unused_preload` will be true only when the resource load will be delayed, and `should_defer` will return true as well. Therefore `ResourceNeedsLoad()` will be false and the resource load is not started. We schedule resource load instead.
I believe it was fine to schedule it in a single `if (is_potentially_unused_preload)` condition. But updated to schedule inside `else if (is_potentially_unused_preload && should_defer)` for the clarity, which guarantees the resource load is not started yet.
- Kouhei Ueno
- Shunya Shishido
- Yoshisato Yanagisawa
Shunya Shishido (Gerrit)
Shunya Shishido added 1 comment![]( "Open in Gerrit")
ScheduleLoadingPotentiallyUnusedPreload(resource);This still schedule a load for resources that should be deferred due to non-potentially_unused_preload reasons.
(e.g. preload scanner's request for <img> in an image-disabled Document -- `should_defer` is false for the kImage-related condition, but this section triggers a load due to is_potentially_unused_preload)
Perhaps ShouldDeferResource should return a three-state enum value (no-defer/defer/defer-and-schedule).
Nice catch, and thanks for the suggestion. I was thinking if we should introduce enum as well. Uploaded another CL to make this change. Could you give a review there? crrev.com/c/5465409
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Yoshisato Yanagisawa
Kouhei Ueno (Gerrit)
Kouhei Ueno voted Code-Review+1![]( "Open in Gerrit")
- Hiroshige Hayashizaki
- Shunya Shishido
- Yoshisato Yanagisawa
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoshisato Yanagisawa (Gerrit)
Yoshisato Yanagisawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yoshisato Yanagisawa
| Code-Review | +1 |
1 comment
Vector<KURL> FrameFetchContext::GetPotentiallyUnusedPreloads() const {returning `const Vector<KURL>&`
or defining `FetchContext::IsPotentiallyUnusedPreloadURL(const KURL& url)`
is better to avoid a copy.
Considering usage in third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc, it might be fine to go with `IsPotentiallyUnusedPreloadURL()`?
Related details
- Hiroshige Hayashizaki
- Shunya Shishido
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 3 comments![]( "Open in Gerrit")
Vector<KURL> FrameFetchContext::GetPotentiallyUnusedPreloads() const {returning `const Vector<KURL>&`
or defining `FetchContext::IsPotentiallyUnusedPreloadURL(const KURL& url)`
is better to avoid a copy.
Yes, `IsPotentiallyUnusedPreloadURL()` may be simpler in this usage. But let me go with `const Vector<KURL>&` as this makes easier to write tests.
void EnableDeferUnusedPreloadForTesting() {note: If anyone has an idea to avoid this approach, please let me know. This is called from tests. `IsPotentiallyUnusedPreload()` internally have a static variable to cache the feature flag state, and that makes the feature status persistently keeps disabled. I don't come up with good solution for that.
resource->FinishAsError(ResourceError::CancelledError(resource->Url()),nit: the existing callers for `ResourceFetcher::StartLoad(Resource*)` don't call FinishAsError() on error.
So this looks a little inconsistent (with those existing callers), but it's probably OK -- anyway this is consistent with Line 1377.
Ack, let me know if you feel some changes are needed.
Related details
- Hiroshige Hayashizaki
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minoru Chikamune (Gerrit)
Minoru Chikamune voted Code-Review+1![]( "Open in Gerrit")
- Hiroshige Hayashizaki
- Shunya Shishido
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hiroshige Hayashizaki
| Code-Review | +1 |
1 comment
if (!kDeferUnusedPreload && !defer_unused_preload_enabled_for_testing_) {nit optional: I thought `base::ScopedFeatureList` can be used, but is this to use `static const bool` here? anyway optional
Related details
- Shunya Shishido
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
WrapWeakPersistent(this), WrapPersistent(resource)));It's better to use `WrapWeakPersistent` and check `resource` is null in `StartLoadAndFinishIfFailed()`, because we don't want to keep-alive and start loading the resource only for this posted task.
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 2 comments![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Commit-Queue | +2 |
2 comments
WrapWeakPersistent(this), WrapPersistent(resource)));It's better to use `WrapWeakPersistent` and check `resource` is null in `StartLoadAndFinishIfFailed()`, because we don't want to keep-alive and start loading the resource only for this posted task.
Done
if (!kDeferUnusedPreload && !defer_unused_preload_enabled_for_testing_) {nit optional: I thought `base::ScopedFeatureList` can be used, but is this to use `static const bool` here? anyway optional
Thanks. Yes, I'd like to keep `kDeferUnusedPreload` as static const from the performance concern.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
16 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
Insertions: 2, Deletions: 2.
@@ -2466,11 +2466,11 @@
freezable_task_runner_->PostTask(
FROM_HERE,
WTF::BindOnce(&ResourceFetcher::StartLoadAndFinishIfFailed,
- WrapWeakPersistent(this), WrapPersistent(resource)));
+ WrapWeakPersistent(this), WrapWeakPersistent(resource)));
}
void ResourceFetcher::StartLoadAndFinishIfFailed(Resource* resource) {
- if (!resource->StillNeedsLoad()) {
+ if (!resource || !resource->StillNeedsLoad()) {
return;
}
if (!StartLoad(resource)) {
```
Change information
[DeferUnusePreload] Preload requests via PostTask if likely unused
This CL changes the preload dipatch timing by using LCPP. From LCPP
hints, ResourceFetcher looks at the potentially unused preload URLs hint
and schedule a task to dispatch resource loading instead of starting
immediately.
This is the basic implementation of this feature. Several ideas to
change the request dispatch timing will be implemented in next CLs.
- M third_party/blink/renderer/core/loader/frame_fetch_context.cc
- M third_party/blink/renderer/core/loader/frame_fetch_context.h
- M third_party/blink/renderer/platform/loader/fetch/fetch_context.h
- M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
- M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h
- M third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc
- M third_party/blink/renderer/platform/loader/testing/mock_fetch_context.h
Code-Review: +1 by Hiroshige Hayashizaki, +1 by Yoshisato Yanagisawa, +1 by Kouhei Ueno, +1 by Minoru Chikamune
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |