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

Load the next scan each time a new scan is viewed #597

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Conversation

annehaley
Copy link
Contributor

Each time a new scan is requested, we queue loading all the frames in that scan, as well as all the frames in the next scan. If the scan has been loaded already, we skip the queue of that scan.
Finding the next scan to queue includes the first scan in the next experiment if the current scan is the last in its experiment.

From @zachmullen via Slack

We basically want a fixed-sized ring buffer of prefetched scans that goes forward to future experiments

How explicitly should we define the ring buffer? We previously didn't worry about clearing cached frames out as we moved along, perhaps we can trust the browser to manage the cleanup. Within this PR, I only implemented the addition of items to the buffer. Do we need to manually remove items?

I'd appreciate extra scrutiny on your review; I'm sure there's much to improve.

@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: cad8a5b
Status: ✅  Deploy successful!
Preview URL: https://c7c5a096.miqa.pages.dev
Branch Preview URL: https://prefetch-ring.miqa.pages.dev

View logs

@zachmullen
Copy link
Collaborator

We'll definitely want to evict old frame data out of the cache. IMO we can do that as soon as they move on to the next scan, or maybe we can keep one previous scan around. If we never evict them, we'll run up against memory limits.

I haven't explored the code much; where is the cache data structure located?

@curtislisle
Copy link
Contributor

curtislisle commented Sep 27, 2022 via email

@zachmullen
Copy link
Collaborator

Thanks for the feedback @curtislisle . I didn't realize going backward and forward would be common -- in that case, a ring buffer makes even more sense as our cache structure.

@curtislisle
Copy link
Contributor

curtislisle commented Sep 27, 2022 via email

@annehaley
Copy link
Contributor Author

I haven't explored the code much; where is the cache data structure located?

The caching structure is complicated, but I think all functional parts are located in store/index.js. We do have map structures for fileCache and frameCache (unsure of the distinction and need for separation...). However we only clear them in the reset action, and otherwise do not reduce their size.
reset in index.js

@zachmullen
Copy link
Collaborator

I think this merits adding a cache abstraction, i.e. functions to mediate adding to the cache that internally handle eviction, rather than direct access to the underlying data structure.

@annehaley
Copy link
Contributor Author

I think this is a good idea, but I'm not sure how to begin. Do you have a similar example or would you mind working with me on this?

@annehaley annehaley mentioned this pull request Sep 27, 2022
@annehaley
Copy link
Contributor Author

From @zachmullen via Slack

unfortunately I won't have time this week to work on a more robust cache / prefetch implementation with you. If what you have solves the immediate issue at hand, I would suggest merging it.

Merging this and opening #598 to track further action

@annehaley annehaley merged commit e67ab4c into master Sep 27, 2022
@annehaley annehaley deleted the prefetch-ring branch September 27, 2022 19:37
zachmullen pushed a commit that referenced this pull request Dec 19, 2022
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