fix: batch caching of remote pin lookups #1741
Conversation
There was a problem hiding this comment.
@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-componentsstill requiresreact-loadableso 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.
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.
lidel
force-pushed
the
chore/optimize-remote-pin
branch
from
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
lidel
force-pushed
the
chore/optimize-remote-pin
branch
from
6207f68
to
7fd0387
Compare
lidel
force-pushed
the
chore/optimize-remote-pin
branch
from
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.
Maybe let's add a github issue with this?
lidel
changed the title
Still WIP