Increase limit for viewport-heuristic-triggered prefetches [chromium/src : main]
Javier Garcia Visiedo (Gerrit)
Javier Garcia Visiedo added 1 comment![]( "Open in Gerrit")
Could you please take a look? Thank you!
Related details
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 1 comment![]( "Open in Gerrit")
Could you separate this CL into two parts: one for a pure increase of prefetch limits, and another for changing viewport heuristics?
Related details
- Hiroki Nakagawa
- Javier Garcia Visiedo
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. |
Javier Garcia Visiedo (Gerrit)
Javier Garcia Visiedo added 1 comment![]( "Open in Gerrit")
Could you separate this CL into two parts: one for a pure increase of prefetch limits, and another for changing viewport heuristics?
- Hiroki Nakagawa
- Takashi Nakayama
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 4 comments![]( "Open in Gerrit")
}`PreloadingAttempt` is an object for logging, not for controlling prefetch operation. Instead, can we take the eagerness from `PrefetchRequest`?
```
size_t limit = kMaxNumberOfNonImmediatePrefetchesPerPage;
if (prefetch->request().prefetch_type().GetEagerness() ==
blink::mojom::SpeculationEagerness::kModerate) {
limit = features::kPrefetchViewportHeuristicsLimitValue.Get();
}
```
completed_non_immediate_prefetches_.front();Let's say that the limit of moderate is 4, while the limit of eager/conservative is 2, and conservative prefetch is triggered after 4 moderate prefetches are triggered. In this scenario, to trigger the conservative prefetch, 3 prefetches need to be cancelled, but in the current implementation, only one oldest prefetch is cancelled. The implementation need to be updated to choose more prefetches to be evicted.
By the way, I wonder if we should also loosen the limit of "eager" prefetches as well to keep the gradation among eagerness? In the current setup, it would be:
- immediate: 50
- eager: 2
- moderate: 4-6?
- conservative: 2
Probably "eager" should have the same limit as "moderate" for now?
- immediate: 50
- eager: 4-6?
- moderate: 4-6?
- conservative: 2
PreloadingPredictor enacting_predictor() const { return enacting_predictor_; }`enacting_predictor()` is also not used if the eagerness is checked via `PrefetchRequest`.
PreloadingPredictor creating_predictor() const { return creating_predictor_; }`creating_predictor()` is not used.
Related details
- Javier Garcia Visiedo
- Takashi Nakayama
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. |
Javier Garcia Visiedo (Gerrit)
Javier Garcia Visiedo added 1 comment![]( "Open in Gerrit")
completed_non_immediate_prefetches_.front();Let's say that the limit of moderate is 4, while the limit of eager/conservative is 2, and conservative prefetch is triggered after 4 moderate prefetches are triggered. In this scenario, to trigger the conservative prefetch, 3 prefetches need to be cancelled, but in the current implementation, only one oldest prefetch is cancelled. The implementation need to be updated to choose more prefetches to be evicted.
By the way, I wonder if we should also loosen the limit of "eager" prefetches as well to keep the gradation among eagerness? In the current setup, it would be:
- immediate: 50
- eager: 2
- moderate: 4-6?
- conservative: 2
Probably "eager" should have the same limit as "moderate" for now?
- immediate: 50
- eager: 4-6?
- moderate: 4-6?
- conservative: 2
Thanks, makes sense. I'll work on the eviction logic to account for the scenario you mentioned.
When it comes to the limits for different levels of eagerness, what about this:
features::kPrefetchViewportHeuristicsLimitValue.Get()
- immediate: 50
- eager: Controlled by features::kPrefetchViewportHeuristicsLimitValue.Get() (4-6)
- moderate: features::kPrefetchViewportHeuristicsLimitValue.Get() (4-6)
- conservative: 2 (default)
With this, we will use the same param for both, eager and moderate. Does this make sense to you? Or do you think we should add different limits (params) for these?
Related details
- Hiroki Nakagawa
- Takashi Nakayama
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 1 comment![]( "Open in Gerrit")
completed_non_immediate_prefetches_.front();Javier Garcia VisiedoLet's say that the limit of moderate is 4, while the limit of eager/conservative is 2, and conservative prefetch is triggered after 4 moderate prefetches are triggered. In this scenario, to trigger the conservative prefetch, 3 prefetches need to be cancelled, but in the current implementation, only one oldest prefetch is cancelled. The implementation need to be updated to choose more prefetches to be evicted.
By the way, I wonder if we should also loosen the limit of "eager" prefetches as well to keep the gradation among eagerness? In the current setup, it would be:
- immediate: 50
- eager: 2
- moderate: 4-6?
- conservative: 2
Probably "eager" should have the same limit as "moderate" for now?
- immediate: 50
- eager: 4-6?
- moderate: 4-6?
- conservative: 2
Thanks, makes sense. I'll work on the eviction logic to account for the scenario you mentioned.
When it comes to the limits for different levels of eagerness, what about this:
features::kPrefetchViewportHeuristicsLimitValue.Get()
- immediate: 50
- eager: Controlled by features::kPrefetchViewportHeuristicsLimitValue.Get() (4-6)
- moderate: features::kPrefetchViewportHeuristicsLimitValue.Get() (4-6)
- conservative: 2 (default)
With this, we will use the same param for both, eager and moderate. Does this make sense to you? Or do you think we should add different limits (params) for these?
Considering the algorithmic difference between "eager" and "moderate", allowing more prefetch for "eager" than for "moderate" seems natural to me. However, since the adoption rate of "eager" eagerness is not high now, keeping on using the same param is a low-cost compromise.
Related details
- Hiroki Nakagawa
- Javier Garcia Visiedo
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. |
Javier Garcia Visiedo (Gerrit)
Javier Garcia Visiedo added 4 comments![]( "Open in Gerrit")
`PreloadingAttempt` is an object for logging, not for controlling prefetch operation. Instead, can we take the eagerness from `PrefetchRequest`?
```
size_t limit = kMaxNumberOfNonImmediatePrefetchesPerPage;
if (prefetch->request().prefetch_type().GetEagerness() ==
blink::mojom::SpeculationEagerness::kModerate) {
limit = features::kPrefetchViewportHeuristicsLimitValue.Get();
}
```
Ack!
Javier Garcia VisiedoLet's say that the limit of moderate is 4, while the limit of eager/conservative is 2, and conservative prefetch is triggered after 4 moderate prefetches are triggered. In this scenario, to trigger the conservative prefetch, 3 prefetches need to be cancelled, but in the current implementation, only one oldest prefetch is cancelled. The implementation need to be updated to choose more prefetches to be evicted.
By the way, I wonder if we should also loosen the limit of "eager" prefetches as well to keep the gradation among eagerness? In the current setup, it would be:
- immediate: 50
- eager: 2
- moderate: 4-6?
- conservative: 2
Probably "eager" should have the same limit as "moderate" for now?
- immediate: 50
- eager: 4-6?
- moderate: 4-6?
- conservative: 2
Takashi NakayamaThanks, makes sense. I'll work on the eviction logic to account for the scenario you mentioned.
When it comes to the limits for different levels of eagerness, what about this:
features::kPrefetchViewportHeuristicsLimitValue.Get()
- immediate: 50
- eager: Controlled by features::kPrefetchViewportHeuristicsLimitValue.Get() (4-6)
- moderate: features::kPrefetchViewportHeuristicsLimitValue.Get() (4-6)
- conservative: 2 (default)
With this, we will use the same param for both, eager and moderate. Does this make sense to you? Or do you think we should add different limits (params) for these?
Considering the algorithmic difference between "eager" and "moderate", allowing more prefetch for "eager" than for "moderate" seems natural to me. However, since the adoption rate of "eager" eagerness is not high now, keeping on using the same param is a low-cost compromise.
Updated the eviction logic.
Also settled on the same limits for eager and moderate for now, both controlled by the feature param.
PreloadingPredictor enacting_predictor() const { return enacting_predictor_; }`enacting_predictor()` is also not used if the eagerness is checked via `PrefetchRequest`.
Removed
PreloadingPredictor creating_predictor() const { return creating_predictor_; }Javier Garcia Visiedo`creating_predictor()` is not used.
Thanks, removed!
Related details
- Hiroki Nakagawa
- Takashi Nakayama
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |