Prefetch: Cancel unrelated prefetch [chromium/src : main]
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
Related details
- Hiroki Nakagawa
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 9 comments![]( "Open in Gerrit")
Thanks!
control it.Can you add VirtualTestSuites for this feature flag for more test coverage?
: public blink::URLLoaderThrottle {Why is this `URLLoaderThrottle`, not `NavigationThrottle`?
Should we create this throttle only if this request is for main frame navigation in a primary page? Otherwise, iframe navigation and subresource requests might cancel prefetches?
->GetPrefetchKeys();We don't need to create `NavigationClaimingPrefetches` when it doesn't exist?
```
if (auto* data = NavigationClaimingPrefetches::GetForNavigationHandle(
*frame_tree_node->navigation_request())) {
prefetch_service->CancelUnrelatedPrefetchForNavigation(
data->GetPrefetchKeys());
}
```
// return &that->web_contents();Was this comment used for debugging? Can we remove this?
// Due to the limitation of `StartPrefetch()`, we can't write a test of theCan we briefly explain the limitation so that code readers don't need to open the crbug below to know it?
// Due to the limitation of `StartPrefetch()` that allows only one prefetch to be triggered, we can't write ...
}`const_cast` is not necessary:
```
const PrefetchContainer* PrefetchService::GetPrefetchContainerForTesting(
const PrefetchKey& key) const {
auto it = owned_prefetches_.find(key);
if (it != owned_prefetches_.end()) {
return it->second.get();
}
return nullptr;
}
```
Related details
- Hiroshige Hayashizaki
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 5 comments![]( "Open in Gerrit")
const base::flat_set<PrefetchKey>& keys) {I suspect `key` is actually not needed, because the `PrefetchContainer` used for serving always has `PrefetchMatchResolverAction::ActionKind::kMaybeServe` in sucessful cases, and thus such PrefetchContainer is already excluded from target.
(Theoretically(?) there might be cases where it was `kMaybeServe` when checking for serving but later failed and no longer `kMaybeServe` here, but anyway probably we can cancel such cases as they anyway are failing)
prefetch_container->GetMatchResolverAction().kind() ==Could you add a comment to explain the intent for this condition?
Perhaps we don't want to cancel prefetches if they already receives successful responses, but not sure/not clear.
PrefetchMatchResolverAction::ActionKind::kMaybeServe ||IIUC this is equivalent to `LoadState::kDeterminedHead` or `LoadState::kCompleted` and I think `LoadState`-based check is much simpler.
keys.contains(prefetch_container->key());Also I think it's better to limit to prefetches with the same initiators to avoid cancelling prefetches from other Documents / other sites.
if (!is_not_target) {```
if (prefetch_container->GetMatchResolverAction().kind() !=
PrefetchMatchResolverAction::ActionKind::kMaybeServe &&
!keys.contains(prefetch_container->key()) {
```
is much easier to read.
Related details
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada voted and added 13 comments![]( "Open in Gerrit")
Votes added by Ken Okada
| Commit-Queue | +1 |
Can you add VirtualTestSuites for this feature flag for more test coverage?
Done
FrameTreeNodeId frame_tree_node_id_;Ken Okadaconst
Done
: public blink::URLLoaderThrottle {Why is this `URLLoaderThrottle`, not `NavigationThrottle`?
- `NavigationRequest::WillStartRequest()` is too early. `NavigationURLLoaderImpl::Start()` and prefetch matching follows it.
- Other hooks are too late.
- Another option was doing something like prefetch matching at, e.g., the head of `NavigationRequest`, rather than utilizing existing prefetch matching. But we (chat with hiroshige@) rejected it to keep PrefetchURLLoaderInterceptor/PrefetchMatchResolver "SSoT" of prefetch matching.
Added as comments.
Should we create this throttle only if this request is for main frame navigation in a primary page? Otherwise, iframe navigation and subresource requests might cancel prefetches?
Thanks! Right.
Done.
We don't need to create `NavigationClaimingPrefetches` when it doesn't exist?
```
if (auto* data = NavigationClaimingPrefetches::GetForNavigationHandle(
*frame_tree_node->navigation_request())) {
prefetch_service->CancelUnrelatedPrefetchForNavigation(
data->GetPrefetchKeys());
}
```
Done with adding else clause.
Was this comment used for debugging? Can we remove this?
Done
// Due to the limitation of `StartPrefetch()`, we can't write a test of theCan we briefly explain the limitation so that code readers don't need to open the crbug below to know it?
// Due to the limitation of `StartPrefetch()` that allows only one prefetch to be triggered, we can't write ...
`StartPrefetch()` allows triggering only one prefetch.
Added a comment.
I suspect `key` is actually not needed, because the `PrefetchContainer` used for serving always has `PrefetchMatchResolverAction::ActionKind::kMaybeServe` in sucessful cases, and thus such PrefetchContainer is already excluded from target.
(Theoretically(?) there might be cases where it was `kMaybeServe` when checking for serving but later failed and no longer `kMaybeServe` here, but anyway probably we can cancel such cases as they anyway are failing)
That's right! Thanks!
Updated.
prefetch_container->GetMatchResolverAction().kind() ==Could you add a comment to explain the intent for this condition?
Perhaps we don't want to cancel prefetches if they already receives successful responses, but not sure/not clear.
Done
PrefetchMatchResolverAction::ActionKind::kMaybeServe ||IIUC this is equivalent to `LoadState::kDeterminedHead` or `LoadState::kCompleted` and I think `LoadState`-based check is much simpler.
Done
keys.contains(prefetch_container->key());Also I think it's better to limit to prefetches with the same initiators to avoid cancelling prefetches from other Documents / other sites.
The purpose of this feature/experiment (at least this phase) is investigating effect of network and other resource congestion on performance metrics. (ParallelPrefetch: increase possibility, CancelUnrelatedPrefetch: decrease.)
So, it's nice to maximize the targets and the effect.
```
if (prefetch_container->GetMatchResolverAction().kind() !=
PrefetchMatchResolverAction::ActionKind::kMaybeServe &&
!keys.contains(prefetch_container->key()) {
```is much easier to read.
Done
`const_cast` is not necessary:
```
const PrefetchContainer* PrefetchService::GetPrefetchContainerForTesting(
const PrefetchKey& key) const {
auto it = owned_prefetches_.find(key);
if (it != owned_prefetches_.end()) {
return it->second.get();
}
return nullptr;
}
```
- Hiroki Nakagawa
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 2 comments![]( "Open in Gerrit")
: public blink::URLLoaderThrottle {Ken OkadaWhy is this `URLLoaderThrottle`, not `NavigationThrottle`?
- `NavigationRequest::WillStartRequest()` is too early. `NavigationURLLoaderImpl::Start()` and prefetch matching follows it.
- Other hooks are too late.
- Another option was doing something like prefetch matching at, e.g., the head of `NavigationRequest`, rather than utilizing existing prefetch matching. But we (chat with hiroshige@) rejected it to keep PrefetchURLLoaderInterceptor/PrefetchMatchResolver "SSoT" of prefetch matching.
Added as comments.
Memo: I'm wondering we should migrate this and `PrerenderURLLoaderThrottle` out of `URLLoaderThrottle` (perhaps by introducing more direct code in `NavigationURLLoaderImpl`), but this can/should be done separately/later not to block the experiment.
"bases": [],Shouldn't we list some bases here?
Related details
- Hiroki Nakagawa
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
"bases": [],Shouldn't we list some bases here?
I'm not so sure as (I think) it's not documented well... Lot's of suite uses `[]` and I guess that it's something like `"ALL"`.
Related details
- Hiroki Nakagawa
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
keys.contains(prefetch_container->key());Ken OkadaAlso I think it's better to limit to prefetches with the same initiators to avoid cancelling prefetches from other Documents / other sites.
The purpose of this feature/experiment (at least this phase) is investigating effect of network and other resource congestion on performance metrics. (ParallelPrefetch: increase possibility, CancelUnrelatedPrefetch: decrease.)
So, it's nice to maximize the targets and the effect.
Chatted.
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki voted and added 5 comments![]( "Open in Gerrit")
Votes added by Hiroshige Hayashizaki
| Code-Review | +1 |
5 comments
GURL url_navigated =nit optional: it's clearer to show the correspondance bewteen the URLs in the code and comment, e.g.
Adding a comment like:
```
// URL V
```
or renaming to `url_V` etc.
ditto below.
// - Start navigation X for URL V.URL U?
// - It doesn't cancel A as it is related.Just to check: How this test is passing? I thought `CancelUnrelatedPrefetchForNavigation()` is called on the initial navigation (i.e. just before the network request to URL V) and thus prefetch A is cancelled there, but surely I'm missing something.
// Cancel unrelated prefetches.nit: `Cancels`.
"bases": [],Ken OkadaShouldn't we list some bases here?
I'm not so sure as (I think) it's not documented well... Lot's of suite uses `[]` and I guess that it's something like `"ALL"`.
I feel running all is too broad -- but anyway I'll defer to virtual test owner.
Related details
- Hiroki Nakagawa
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa voted and added 3 comments![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
3 comments
: public blink::URLLoaderThrottle {Ken OkadaWhy is this `URLLoaderThrottle`, not `NavigationThrottle`?
Hiroshige Hayashizaki
- `NavigationRequest::WillStartRequest()` is too early. `NavigationURLLoaderImpl::Start()` and prefetch matching follows it.
- Other hooks are too late.
- Another option was doing something like prefetch matching at, e.g., the head of `NavigationRequest`, rather than utilizing existing prefetch matching. But we (chat with hiroshige@) rejected it to keep PrefetchURLLoaderInterceptor/PrefetchMatchResolver "SSoT" of prefetch matching.
Added as comments.
Memo: I'm wondering we should migrate this and `PrerenderURLLoaderThrottle` out of `URLLoaderThrottle` (perhaps by introducing more direct code in `NavigationURLLoaderImpl`), but this can/should be done separately/later not to block the experiment.
That makes sense, thanks!
"bases": [],Ken OkadaShouldn't we list some bases here?
Hiroshige HayashizakiI'm not so sure as (I think) it's not documented well... Lot's of suite uses `[]` and I guess that it's something like `"ALL"`.
I feel running all is too broad -- but anyway I'll defer to virtual test owner.
Should we run this only for speculation rules (external/wpt/speculation-rules/), as other WPTs don't trigger speculation rules prefetch in the first place?
Related details
- Ken Okada
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hiroshige Hayashizaki
| Code-Review | +1 |
2 comments
// - Start navigation X for URL V.Hiroshige HayashizakiURL U?
Done
// Cancel unrelated prefetches.Hiroshige Hayashizakinit: `Cancels`.
Done
Related details
- Hiroki Nakagawa
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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")
"bases": [],Ken OkadaShouldn't we list some bases here?
Hiroshige HayashizakiI'm not so sure as (I think) it's not documented well... Lot's of suite uses `[]` and I guess that it's something like `"ALL"`.
Hiroki NakagawaI feel running all is too broad -- but anyway I'll defer to virtual test owner.
Should we run this only for speculation rules (external/wpt/speculation-rules/), as other WPTs don't trigger speculation rules prefetch in the first place?
+1 for just `external/wpt/speculation-rules` unless we have specific reasons.
Ken Okada (Gerrit)
Ken Okada added 5 comments![]( "Open in Gerrit")
: public blink::URLLoaderThrottle {Ken OkadaWhy is this `URLLoaderThrottle`, not `NavigationThrottle`?
Hiroshige Hayashizaki
- `NavigationRequest::WillStartRequest()` is too early. `NavigationURLLoaderImpl::Start()` and prefetch matching follows it.
- Other hooks are too late.
- Another option was doing something like prefetch matching at, e.g., the head of `NavigationRequest`, rather than utilizing existing prefetch matching. But we (chat with hiroshige@) rejected it to keep PrefetchURLLoaderInterceptor/PrefetchMatchResolver "SSoT" of prefetch matching.
Added as comments.
Hiroki NakagawaMemo: I'm wondering we should migrate this and `PrerenderURLLoaderThrottle` out of `URLLoaderThrottle` (perhaps by introducing more direct code in `NavigationURLLoaderImpl`), but this can/should be done separately/later not to block the experiment.
That makes sense, thanks!
Done
GURL url_navigated =nit optional: it's clearer to show the correspondance bewteen the URLs in the code and comment, e.g.
Adding a comment like:
```
// URL V
```or renaming to `url_V` etc.
ditto below.
Updated as "URL `unl_navigated`" etc.
// - Start navigation X for URL V.URL U?
Done
Just to check: How this test is passing? I thought `CancelUnrelatedPrefetchForNavigation()` is called on the initial navigation (i.e. just before the network request to URL V) and thus prefetch A is cancelled there, but surely I'm missing something.
Ah, good catch. Sorry. This was passed because the prefetch is servable as the test waits the prefetch. Using ControllableHttpResponse, it fails.
Updated expectation. Unfortunately, this case is unavoidable.
Chatted and mark resolved.
// Cancel unrelated prefetches.nit: `Cancels`.
- Hiroki Nakagawa
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
"bases": [],Ken OkadaShouldn't we list some bases here?
Hiroshige HayashizakiI'm not so sure as (I think) it's not documented well... Lot's of suite uses `[]` and I guess that it's something like `"ALL"`.
Hiroki NakagawaI feel running all is too broad -- but anyway I'll defer to virtual test owner.
Hiroshige HayashizakiShould we run this only for speculation rules (external/wpt/speculation-rules/), as other WPTs don't trigger speculation rules prefetch in the first place?
+1 for just `external/wpt/speculation-rules` unless we have specific reasons.
https://chromium-review.git.corp.google.com/c/chromium/src/+/7792117
My understanding was wrong. Added external/wpt/speculation-rules and http.
Related details
- Hiroki Nakagawa
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
1 comment
"bases": ["external/wpt/speculation-rules", "http"],Which tests do we want to run under "http"? Is it possible to narrow down the scope like "external/wpt/speculation-rules"?
Related details
- Hiroshige Hayashizaki
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
"bases": ["external/wpt/speculation-rules", "http"],Which tests do we want to run under "http"? Is it possible to narrow down the scope like "external/wpt/speculation-rules"?
E.g. http/tests/inspector-protocol/prefetch. Narrowed down to http/tests/inspector-protocol.
Related details
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |