-
Notifications
You must be signed in to change notification settings - Fork 42
Update "wait for a matching prefetch record" #411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* The note stating that this would allow using prefetches that started after the navigation has not been accurate, since 24b2405 created a snapshot of the records. As such, the whole cutoff time machinery was not useful and could be deleted. * Add an optional timeout, with a note suggesting it's best used sparingly. Closes #389.
| Preview: |
| <p class="note" id="note-wait-for-matching-prefetch-record-timeout">Choosing a good timeout is difficult, and might depend on the exact initiator of the navigation or the prefetch (e.g., its [=speculation rule eagerness=], or whether it will be used for a prerender). In general, a timeout is best avoided, as it is rare that restarting the navigation as a non-prefetch will give a better result than awaiting the ongoing prefetch. But some implementations have found that in some situations, a timeout on the order of seconds can give better results.</p> | ||
| 1. Let |startTime| be the [=unsafe shared current time=]. | ||
| 1. Run these steps, but [=abort when=] |timeout| is not null and the [=unsafe shared current time=] − |startTime| is greater than |timeout|: | ||
| 1. While true: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
1. [=Assert=]: this is running [=in parallel=].
1. Let |timeout| be null.
1. Optionally, set |timeout| to an [=implementation-defined=] duration representing the maximum time the implementation is willing to wait for an ongoing prefetch to respond, before it gives up and restarts the navigation.
<p class="note" id="note-wait-for-matching-prefetch-record-timeout">Choosing a good timeout is difficult, and might depend on the exact initiator of the navigation or the prefetch (e.g., its [=speculation rule eagerness=], or whether it will be used for a prerender). In general, a timeout is best avoided, as it is rare that restarting the navigation as a non-prefetch will give a better result than awaiting the ongoing prefetch. But some implementations have found that in some situations, a timeout on the order of seconds can give better results.</p>
1. Let |startTime| be the [=unsafe shared current time=].
1. Let |completeRecord| be the result of [=finding a matching complete prefetch record=] given |navigable|, |sourceSnapshotParams|, and |url|.
1. If |completeRecord| is not null, return |completeRecord|.
1. Let |potentialRecords| be an empty [=list=].
1. [=list/For each=] |record| of |sourceSnapshotParams|'s [=source snapshot params/prefetch records=]:
1. If all of the following are true:
* |record|'s [=prefetch record/state=] is "`ongoing`";
* |record| [=prefetch record/is expected to match a URL=] given |url|; and
* |record|'s [=prefetch record/expiry time=] is greater than the [=current high resolution time=] for |navigable|'s [=navigable/active window=],
then [=list/append=] |record| to |potentialRecords|.
1. Run these steps, but [=abort when=] |timeout| is not null and the [=unsafe shared current time=] − |startTime| is greater than |timeout|:
1. While true:
1. If |potentialRecords| [=list/is empty=], return null.
1. Wait until the [=prefetch record/state=] of any element of |sourceSnapshotParams|'s [=source snapshot params/prefetch records=] changes.
1. Let |completeRecord| be the result of [=finding a matching complete prefetch record=] given |navigable|, |sourceSnapshotParams|, and |url|.
1. If |completeRecord| is not null,
1. If |completeRecord| is not in |potentialRecords|, return null. (*)
1. Return |completeRecord|.
1. [=list/For each=] |record| of |potentialRecords|:
1. If |records|'s [=prefetch record/state=] is not "ongoing", [=list/remove=] |record| from |potentialRecords|.
Because
potentialRecordsshould be initialized once.potentialRecordsmust decrease to fulfillIf |potentialRecords| [=list/is empty=], return null..
Note that
Let |completeRecord| be the result of [=finding a matching complete prefetch record=]...appears twice, as the conditions of return are slightly different to ensureIf |completeRecord| is not in |potentialRecords|, return null.. We can improve it by "collect potentially or actually matching records, and then find "completed" ones".- The last
If |records|'s [=prefetch record/state=] is not "ongoing", [=list/remove=] |record| from |potentialRecords|.handles two cases: 1. It removes "canceled" ones. 2. It removes ones that become "completed" but the NVS header is not compatible to the NVS hint. - (*) is compromised. It's slightly different to what the Chrome's implementation intended. Actually, we should iterate
potentialRecordsand check "completed" ones for it. But I guess we need another helper to check NVS condition. (One way is add a pure version of #find-a-matching-complete-prefetch-record and apply it toprefetchRecords.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non-blocking memo) I'm slightly negative about this:
- Pros: This is a little more aligned with Chromium implementation structure.
- Cons: The behavior is equivalent (am I understanding correctly?), and this adds duplicated logic into the spec (two
[=finding a matching complete prefetch record=]calls and two"ongoing"checks), which looks like unnecessary optimization in the spec and is a little more error-prone.
The main added benefit by the proposal would be clarifying that potentialRecords is monotonically decreasing over time.
This is already the case in the domenic's original version.
(i.e. each of the three conditions at Lines 323-325 never turns from false to true over time, and the |sourceSnapshotParams|'s [=source snapshot params/prefetch records=] isn't chaning over time)
We can add a (non-normative) note to clarify this monotonically decreasing nature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the careful review and the detailed suggestion!
I read through both comments here carefully and I think I end up agreeing with @hiroshige-g. As long as the behaviors are equivalent, my current version is a bit easier to read. It is less efficient, since it reconstructs a new potentialRecords list each time instead of mutating a single list. But specifications tend to focus on being easy to read instead of being optimized.
I think my current version also avoids the compromise you mention at (*).
I do agree that we should make it clearer that each new iteration of potentialRecords will be monotonically decreasing, so I will add a note to that effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they are not equivalent. Consider the case sourceSnapshotPraam's prefetch records are changed (e.g. added a new entry), so potentialRecords can increase.
If it's intended, it's OK, but it's not equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the case
sourceSnapshotPraam's prefetch records are changed
They cannot change: they are a snapshot. This is explained in the new note I added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me merge this, since I am retiring, @kenoss is OOO, and I believe they are equivalent. If I have missed something, please open a new issue and @domfarolino and @hiroshige-g can help resolve it.
|
non-owner LGTM, deferring to domenic/kenoss. |
The note stating that this would allow using prefetches that started after the navigation has not been accurate, since 24b2405 created a snapshot of the records. As such, the whole cutoff time machinery was not useful and could be deleted.
Add an optional timeout, with a note suggesting it's best used sparingly.
Closes #389.
@kenoss can you take a look?