Skip to content
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

[spec] Update saved query to be async #202

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

pythagoraskitty
Copy link
Collaborator

@pythagoraskitty pythagoraskitty commented Oct 7, 2024

We fix our spec changes so that a query can be queued to reused if it is not the initial query but it is received before the initial query is resolved.

Previously in the spec, only fully resolved queries were being treated as saved. This was an oversight.


Preview | Diff

We fix our spec changes so that a query can be queued to reused if it is not the initial query but it is received before the initial query is resolved.

Previously, only fully resolved queries were being treated as saved.
@pythagoraskitty
Copy link
Collaborator Author

@xyaoinum and @wanderview Please take a look, thanks!

Copy link
Collaborator

@xyaoinum xyaoinum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Have one question about the behavior for when the first selectURL() is rejected.

spec.bs Show resolved Hide resolved
spec.bs Show resolved Hide resolved
@xyaoinum
Copy link
Collaborator

xyaoinum commented Oct 9, 2024

Two more questions on budget charging:

  1. How did you avoid double-charging the per-page budget when returning a saved state? It looks like they will all end up in step "33. Upon fulfillment of indexPromise with resultIndex ...", which will always charge the budget. Did I miss anything?
  2. It appears that the budget is not being charged in "34. Upon rejection of indexPromise, ...", which seems incorrect and a pre-existing bug? Suggest either filing a separate issue to track, or fixing it directly here. The fix seems simple: just mirror the logic used when the promise is fulfilled % needing to use default index 0.

@pythagoraskitty
Copy link
Collaborator Author

pythagoraskitty commented Oct 9, 2024

Two more questions on budget charging:

  1. How did you avoid double-charging the per-page budget when returning a saved state? It looks like they will all end up in step "33. Upon fulfillment of indexPromise with resultIndex ...", which will always charge the budget. Did I miss anything?

You're correct; there were some bugs in my logic that I've now fixed. First, i forgot to return |promise| and abort in the case that we already had a saved index available (I implied that this is what I wanted done but didn't mention it explicitly.) Second, I was still charging the budgets, as you noted. Now I will use a tuple with a boolean to keep track of whether or not to charge the budgets.

  1. It appears that the budget is not being charged in "34. Upon rejection of indexPromise, ...", which seems incorrect and a pre-existing bug? Suggest either filing a separate issue to track, or fixing it directly here. The fix seems simple: just mirror the logic used when the promise is fulfilled % needing to use default index 0.

This issue I don't quite follow. Why do we need to charge budget in the rejection case? Won't the charge be 0 bits? What did I miss?

@xyaoinum
Copy link
Collaborator

xyaoinum commented Oct 9, 2024

LGTM % the rejection case question

This issue I don't quite follow. Why do we need to charge budget in the rejection case? Won't the charge be 0 bits? What did I miss?

If we don't charge the budget for reject, then if selectURL() happens to return 0, it could instead trigger an error to achieve the desired outcome without incurring budget costs. This issue was previously addressed in the code: https://crrev.com/c/3846383 (which seems to occur before we had the spec)

@pythagoraskitty
Copy link
Collaborator Author

LGTM % the rejection case question

This issue I don't quite follow. Why do we need to charge budget in the rejection case? Won't the charge be 0 bits? What did I miss?

If we don't charge the budget for reject, then if selectURL() happens to return 0, it could instead trigger an error to achieve the desired outcome without incurring budget costs. This issue was previously addressed in the code: https://crrev.com/c/3846383 (which seems to occur before we had the spec)

I had forgotten that we had done that. Thanks for reminding me. I will fix the rejection case now in the spec.

@pythagoraskitty
Copy link
Collaborator Author

@xyaoinum Please take another look, as I think I fixed the rejection bug. Thanks!

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
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.

3 participants