feat: handle incoming blob filters #956
Conversation
This change should have no impact.
…Stream` This is a types-only change that should have no functionality impact.
This change should have no impact on functionality. It makes the following changes to `PeerState`: - Instead of a `#wants` bitfield and a `#wantAll` boolean, we only store one value: `#wants`, which can be `null` or a bitfield. This helps avoid states where you want everything *and* the bitfield has data inside. - `setWantRange` is renamed to `addWantRange`. - `addWantRange` takes two numbers (a start and length) instead of an object. This is for performance and consistency with future code. - `clearWantRanges` is a new method that resets the bitfield. I think these are useful changes on their own, but will make upcoming changes smaller too.
This should have no functionality impact.
We weren't doing this when `SyncApi` was instantiated. Now we are.
EvanHahn
force-pushed
the
want-bitfield
branch
from
70f65b4
to
7c810c2
Compare
| */ | ||
| setPeerWants(peerId, ranges) { | ||
| addWantRange(peerId, start, length) { |
There was a problem hiding this comment.
I was worried about performance (because of the cost of calculating state), but I think the way this is implemented, the throttle on SyncState should avoid state calculation happening too frequently, so I think it is ok to just call this with every range (there will be a lot of calls to this).
There was a problem hiding this comment.
I wrote a simple benchmark script with Deno. On my machine, I could call setRange 7,364 times per second, at least with my simple benchmark. I realize my computer is probably much faster than most phones, but even a 100x slowdown is probably okay here.
I'm going to leave this as is, but let me know if that's wrong.
There was a problem hiding this comment.
Sorry for the lack of clarity, I was thinking out loud. I was concerned about the performance not because of the cost of this function, but because it triggers a state update, and reading state is expensive (when cores get large, which they will do for the blob core). However, the way we currently trigger state updates, we don't actually calculate state as part of the update event, and then in SyncState we throttle the handler for update events so that getState() is only called ever 200ms, so with the current implementation this is all fine. The reason this was originally ranges was to avoid triggering a state-recalculation for every range set, but our throttle in SyncState solves that problem for now. We just need to remember this in the future.
I recommend reviewing this PR one commit at a time.
When you receive a blob filter from another peer, this updates their sync states. For example, if you receive a blob filter that says "I only want photo thumbnails", that peer's "wants" bitfield will be reduced.
Closes #682 and #905.