Skip to content

Conversation

@domenic
Copy link
Collaborator

@domenic domenic commented Sep 11, 2025

  • 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?

* 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.
@domenic domenic requested a review from domfarolino September 11, 2025 07:25
@github-actions
Copy link

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=] &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.

@hiroshige-g
Copy link
Collaborator

non-owner LGTM, deferring to domenic/kenoss.

@domenic domenic merged commit e8e5e5c into main Sep 26, 2025
2 checks passed
@domenic domenic deleted the wait-for-matching branch September 26, 2025 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Wait for a matching prefetch record" doesn't quite match Chromium's implementation

3 participants