-
Notifications
You must be signed in to change notification settings - Fork 235
fix(mongodb-compass): Do not overfetch connectionInfo
and do not update state too often COMPASS-5327
#2618
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
Conversation
…n fetching db/coll list to avoid overfetching authInfo
…n global overlay is active
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.
👍👍👍
getDatabasesAndCollectionsFromPrivileges(this._initializedClient, [ | ||
'find', | ||
]).then((databases) => { | ||
(privileges |
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.
Not really important, did you consider having this code in some local named closure or private method? Is getting a bit intense to read it inside the Promise.all
.
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.
Yeah, I was going back and forth on this when implementing and I think the fact it caught your eye suggests that I probably should move it out actually 😄
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.
(collections, databases, dbName = store.getState().databaseName) => { | ||
collections = collections ?? databases.get(dbName)?.collections; | ||
if (collections && collections.parent.getId() === dbName) { | ||
const onCollectionsChange = throttle((collections, force = false) => { |
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.
how does this work? are simultaneous invocations discarded or enqueued? Like could it be that we miss an update here?
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.
First one and the last one always fires, everything in between is throttled by dropping them, but firing the closest one to the throttled time span (so like every 100ms one will fire). We will miss some updates, but will get the first and the last state which is the most important ones. The ones we loose are also not that important here (nor in the databases-store), they will be intermediate dbStats / collStats populating data in separate database / collection instances.
Generally speaking I would prefer us not to have any throttling but it helps to mitigate the issues with React reconciling too much when state updates happen for now. Does this sounds alright to you?
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.
Sounds great yeah! thanks for the explanation
3728b41
to
b791155
Compare
This was initially reported to the MongoDB Support (see this thread for additional details). People using Compass 1.29.5 would randomly get performance issues when using the app. Sometimes it will connect and show databases normally, sometimes it would be stuck on the "Loading databases" screen for a long time or even indefinitely.
I did a few performance profiles and compared the data-service behavior between the two versions and found two differences that we introduced with the recent changes to the
data-service
andinstance-model
store subscriptions even though we tried to keep the behavior with the global overlay as close to the old one as possible with the help of theCOMPASS_NO_GLOBAL_OVERLAY
environmental flag. Following things changed and caused a performance regressionOverfetching
connectionStatus
We started fetching
connectionStatus
info way too often. User privileges returned by theconnectionStatus
command are used to get databases and collections names in some cases and anticipating the future logic where collections for every database would be fetched only when needed, we moved this logic out ofinstance
method and into listDatabases / listCollections methods. So when global overlay was disabled, this additional time spent fetchingconnectionStatus
was not affecting anything much, but with the global overlay when we are prefetching all the data in advance it had a significant impact.This PR fixes the issue by allowing to pass privileges to the
list
methods and changes models to get privileges from instance model and pass them to thelist
methods to avoid fetching them too often.Too many state updates
Another issue that wasn't obvious immediately but became very clear after doing a bit of profiling was that due to how the stores are subscribed to the models now and the fact that almost all Compass plugins are passing too much state to the views even when it's not used or needed, some of those constant updates were overloading React and caused giant performance issues, especially for the databases-collections view that is not virtual and renders all of the items in the list at once.
This is addressed by splitting the update logic with the env var in the relevant stores and changing it back to what it was before (it's mostly updating just once when all database stats or collection stats are loaded)
I tested it with a locally running server with 10k databases and 15k collections and it works alright for me: