⚡️ Cache latest op version when broadcasting presence#679
Merged
Conversation
788b18f to
e247940
Compare
389e06e to
5c9994d
Compare
At the moment, when sending a presence update to other subscribers, we [call `transformPresenceToLatestVersion()`][1] for every presence update which internally [calls `getOps()`][2] for every presence update. Calls to `getOps()` can be expensive, and rapid presence updates may cause undue load on the server, even when the `Doc` has not been updated. This change tries to mitigate this by subscribing to a pubsub stream for any `Doc`s that an `Agent` tries to broadcast presence on. We keep an in-memory cache of the latest snapshot version sent over this stream, which lets us quickly check if a presence broadcast is already current without needing to query the database at all. To avoid leaking streams, the `Agent` will internally handle its stream subscription state by: - subscribing whenever a non-`null` presence update is broadcast - unsubscribing whenever a `null` presence update is broadcast This means that rapid changes in presence being `null` or not can still result in database calls, but even in this case they should be less bad than before, because we only perform a snapshot fetch instead of ops. [1]: https://github.com/share/sharedb/blob/297ce5dc66563a5955311793a475768d73ac8b87/lib/agent.js#L804 [2]: https://github.com/share/sharedb/blob/297ce5dc66563a5955311793a475768d73ac8b87/lib/backend.js#L919
5c9994d to
87ef11d
Compare
ericyhwang
reviewed
Aug 27, 2024
| }; | ||
|
|
||
| Agent.prototype._unsubscribeDocVersion = function(collection, id, callback) { | ||
| var stream = util.dig(this.latestDocVersionStreams, collection, id); |
Contributor
There was a problem hiding this comment.
This doesn't clean up the this.latestDocVersions, which means a long-lived client that goes through many docs could have an agent with tons of old entries in that map.
Per discussion, immediately cleaning up this.latestDocVersions may make the case of multiple null presences worse. Perhaps a delayed cleanup or something.
- Clean up `latestDocVersions` when unsubscribing - Fix not setting version if the object already exists - Removes `null` presence check, which won't work if the object has been destroyed
ericyhwang
approved these changes
Aug 27, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At the moment, when sending a presence update to other subscribers, we call
transformPresenceToLatestVersion()for every presence update which internally callsgetOps()for every presence update.Calls to
getOps()can be expensive, and rapid presence updates may cause undue load on the server, even when theDochas not been updated.This change tries to mitigate this by subscribing to a pubsub stream for any
Docs that anAgenttries to broadcast presence on. We keep an in-memory cache of the latest snapshot version sent over this stream, which lets us quickly check if a presence broadcast is already current without needing to query the database at all.To avoid leaking streams, the
Agentwill internally handle its stream subscription state by:nullpresence update is broadcastnullpresence update is broadcastThis means that rapid changes in presence being
nullor not can still result in database calls, but even in this case they should be less bad than before, because we only perform a snapshot fetch instead of ops.