-
Notifications
You must be signed in to change notification settings - Fork 484
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
fix: batch caching of remote pin lookups #1741
Conversation
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.
@rafaelramalho19 I've played a bit with this PR, and it mostly worked as expected: – no longer fetching all remote pins 🎉 :))
The only missing part was avoiding sending check for CIDs that were already probed,
so I've added some optimizations in 9792b47:
- It adds new cache for "not-pins" per each remote service, so we can remember remote pin state ("pinned" or "not pinned") of each item and avoid sending an unnecessary request every time we see the same CID anywhere in Files.
If you could take a look at remaining TODO here:
- On the initial load, Files screen does not run the check for MFS root. One needs to go to a subdirectory to trigger remote pin checks, and then if one goes back to MFS root, the check is finally executed. We are probably missing
doFetchRemotePins(<MFS root files>)
somehwere, or it gets skipped because some state is not ready yet. -
ipld-explorer-components
still requiresreact-loadable
so either you need to add it back for now, or refactor that lib as well.
Is this still happening? Can't replicate 🤔 |
I am able to reproduce this in both Firefox and Brave with 4 services (only one online) |
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.
Ok, managed to fix the problem. It manifested on my box because I use Firefox and have a loooot files in the root directory, so initial remote pin check was slow enough to trigger performance issues and race condition due to unnecessary rendering.
6207f68 simplifies the way we pass information about remote pins to cache by batching all pin updates into a single event.
This way we avoid delays in UI caused by unnecessary re-renders.
Additional fixes:
- requests are only sent to services that are confirmed to be online
- CIDs to check are deduplicated (in case same item is present multiple times)
- remote pins are created with
background:true
- we don't have UI for "ongoing pin" nor "failed pin" anyway
- above can be added in the future, but it is best-effort for now
- initial load of Files shows remote pins, but we fetch remote pins only if at least one service is online
@rafaelramalho19 looks ok to merge on my end, but please take a look and give 👍 if no concerns.
892f1ce
to
6207f68
Compare
Ugh, I've merged #1644 and now we need to resolve conflicts. |
This adds cache for not-pins per service, so we can cache remote pin state after initial check and avoid sending unnecessary request every time we enter the same directory. TODO: on the initial load, Files screen does not run the check for MFS root. One needs to go to a subdirectory to trigger it, and then if one goes back to MFS root.
This simplifies the way we pass information about remote pins to cache. By batching all pin updates into a single event we avoid delays in UI caused by unnecessary re-renders. Additional fixes: - requests are only sent to services that are confirmed to be online - CIDs to check are deduplicated - initial load of Files has fast remote pin feedback
6207f68
to
7fd0387
Compare
7fd0387
to
1af54f9
Compare
@rafaelramalho19 resolved conflicts – give this a quick look and 👍 / 👎 in a spare moment. |
}) | ||
await ipfs.pin.remote.add(cid, { service: service.name, name }) | ||
adds.push({ id, ...pin }) | ||
// TODO: remove background:true and add pin job to queue. |
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.
Maybe let's add a github issue with this?
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.
Still WIP