Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 27 additions & 16 deletions prefetch.bs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ spec: html; urlPrefix: https://html.spec.whatwg.org/multipage/
text: cross-origin prefetch IP anonymization policy; url: cross-origin-prefetch-ip-anonymization-policy
text: prefetch candidate; url: prefetch-candidate
text: prefetch IP anonymization policy; url: prefetch-ip-anonymization-policy
text: speculation rule eagerness; url: speculation-rule-eagerness
text: speculation rule tag; url: speculation-rule-tag
text: speculative load candidate; url: speculative-load-candidate
for: cross-origin prefetch IP anonymization policy
Expand Down Expand Up @@ -309,22 +310,32 @@ The user agent may [=prefetch record/cancel and discard=] records from the [=Doc
To <dfn export>wait for a matching prefetch record</dfn> given a [=top-level traversable=] |navigable|, [=source snapshot params=] |sourceSnapshotParams|, and [=URL=] |url|:

1. [=Assert=]: this is running [=in parallel=].
1. Let |cutoffTime| be null.
1. While true:
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, then [=list/append=] |record| to |potentialRecords|:
* |record|'s [=prefetch record/state=] is "`ongoing`".
* |record| [=prefetch record/is expected to match a URL=] given |url|.
* |record|'s [=prefetch record/expiry time=] is greater than the [=current high resolution time=] for |navigable|'s [=navigable/active window=].
* |cutoffTime| is null or |record|'s [=prefetch record/start time=] is less than |cutoffTime|.
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. If |cutoffTime| is null and any element of |potentialRecords| has a [=prefetch record/state=] that is not "`ongoing`", set |cutoffTime| to the [=current high resolution time=] for |navigable|'s [=navigable/active window=].

<p class="note">The reasoning for setting the cutoff time *after* waiting for a prefetch record to finish is to allow for flexibility in selecting a prefetch to serve the navigation while still guaranteeing falling back to a non-prefetched navigation in the case of repeated prefetch failures. We allow blocking on prefetch attempts which started before we see an attempt fail, but we don't block on subsequent attempts. Notably, this approach: does not finalize the set of prefetches to block on at the start of the navigation; allows a prefetch which started and completed after the navigation started to serve the navigation; avoids the use of a fixed timeout, which would be arbitrary and detrimental to the use of prefetch with slower servers; and blocks on, at most, two nearly-consecutive prefetches before falling back to a conventional navigation.</p>
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. Run these steps, but [=abort when=] |timeout| is not null and the [=unsafe shared current time=] &minus; |startTime| is greater than |timeout|:
1. While true:
Copy link

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=] &minus; |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

  • potentialRecords should be initialized once.
  • potentialRecords must decrease to fulfill If |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 ensure If |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 potentialRecords and 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 to prefetchRecords.)

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

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|.

<div class="note" id="note-wait-for-matching-prefetch-record-potential-records">
<p>Each iteration of the loop recomputes |potentialRecords|, in a way so that a subsequent iteration's |potentialRecords| will be a subset of the previous iteration's |potentialRecords|. This follows from how |sourceSnapshotParams|'s [=source snapshot params/prefetch records=] is a static snapshot, and none of these three conditions can flip from false to true.</p>

<p>An equivalent strategy would be to build a single initial instance of |potentialRecords|, and remove items from it as they no longer meet the criteria.</p>
</div>
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. Return null.

<p class="note" id="note-wait-for-matching-prefetch-record-subtleties">Because |sourceSnapshotParams|'s [=source snapshot params/prefetch records=] are a snapshot, prefetches that start after a navigation cannot be activated as part of that navigation.</p>
</div>

<div algorithm>
Expand Down