[WIP] Comments server-side CPU usage by jimrandomh · Pull Request #765 · ForumMagnum/ForumMagnum

jimrandomh · 2018-07-30T02:38:32Z

This patch improves performance loading posts with large numbers of comments, especially server-side CPU usage. It also contains some infrastructure for dealing with the N+1-select problem in general, except that it needs some refactoring (I'm putting this up for feedback on the underlying solution, acknowledging that it's very much in the wrong place and has not quite the right API).

There is a change in the Vulcan repository which goes with this one, which is at LessWrong2/Vulcan#5 . This branch also adds a missing index, and load number of votes instead of lists of votes.

…ients

… commentsServerSideCPU

Discordius · 2018-07-30T03:19:49Z

Hmm, my intuition says this is too complicated, and there must be a better way to do this, but quite plausible/likely that I am wrong.

Want to give a bit more background on what the idea behind the custom loaders is, and why it's necessary?

jimrandomh · 2018-07-30T03:46:49Z

As far as I can tell, this (or something very similar to it) is definitely unavoidable if we want non-terrible performance on most things that look like joins. It isn't possible to make currentUserVote performant with its current API in any other way; getting rid of currentUserVote in order to replace it with something join-free would've meant rewriting most of the voting system, and I expect the result would not be pretty.

The purpose of a DataLoader is to batch a set of related queries together. So if you have, say, a set of 500 comments, each of which needs to bring along the User who posted it, you use a DataLoader instead of querying MongoDB directly, and it gathers them up into an {$in:[...]} query. DataLoader is a standalone library which isn't linked to Meteor or even to MongoDB; it just takes... something, and a callback to request all the somethings at once.

At the start of each request, Apollo makes a DataLoader for each collection, which groups together lookup-object-by-ID queries. This is great if what you have is IDs, but useless if what you have is some other batchable query, like the cases of allVotes and currentUserVote.

Discordius · 2018-07-30T04:55:21Z

I do generally prefer to avoid joins, and I don't think we actually need a server-side join in this case. Couldn't we just do a separate non-blocking query for all the votes, and then enhance the store on the client side as soon as that one completes? That seems like the correct way to do it in terms of querying hierarchy, and would allow us to make use of all the standard infrastructure that Vulcan has without needing to change stuff super much. I guess we would have to write some logic to enhance the store, which might turn out to be similarly complicated, but it does feel more isolated than trying to do fancy stuff with the resolvers.

…

On Sun, Jul 29, 2018 at 8:46 PM James Babcock ***@***.***> wrote: As far as I can tell, this (or something very similar to it) is definitely unavoidable if we want non-terrible performance on most things that look like joins. It isn't possible to make currentUserVote performant with its current API in any other way; getting rid of currentUserVote in order to replace it with something join-free would've meant rewriting most of the voting system, and I expect the result would not be pretty. The purpose of a DataLoader is to batch a set of related queries together. So if you have, say, a set of 500 comments, each of which needs to bring along the User who posted it, you use a DataLoader instead of querying MongoDB directly, and it gathers them up into an {$in:[...]} query. DataLoader is a standalone library which isn't linked to Meteor or even to MongoDB; it just takes... something, and a callback to request all the somethings at once. At the start of each request, Apollo makes a DataLoader for each collection, which groups together lookup-object-by-ID queries. This is great if what you have is IDs, but useless if what you have is some other batchable query, like the cases of allVotes and currentUserVote. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#765 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIq25ZXU7grexzGP7HeZ42rYTgdumkzvks5uLoGrgaJpZM4Vlsfv> .

Discordius · 2018-07-30T04:56:33Z

The other alternative would be to just completely replace the CommentsList query with a custom resolver, and there just do a Mongo aggregate query, which does also seem like a sensible way to use the existing abstractions.

…

On Sun, Jul 29, 2018 at 9:55 PM Oliver Habryka ***@***.***> wrote: I do generally prefer to avoid joins, and I don't think we actually need a server-side join in this case. Couldn't we just do a separate non-blocking query for all the votes, and then enhance the store on the client side as soon as that one completes? That seems like the correct way to do it in terms of querying hierarchy, and would allow us to make use of all the standard infrastructure that Vulcan has without needing to change stuff super much. I guess we would have to write some logic to enhance the store, which might turn out to be similarly complicated, but it does feel more isolated than trying to do fancy stuff with the resolvers. On Sun, Jul 29, 2018 at 8:46 PM James Babcock ***@***.***> wrote: > As far as I can tell, this (or something very similar to it) is > definitely unavoidable if we want non-terrible performance on most things > that look like joins. It isn't possible to make currentUserVote > performant with its current API in any other way; getting rid of > currentUserVote in order to replace it with something join-free would've > meant rewriting most of the voting system, and I expect the result would > not be pretty. > > The purpose of a DataLoader is to batch a set of related queries > together. So if you have, say, a set of 500 comments, each of which needs > to bring along the User who posted it, you use a DataLoader instead of > querying MongoDB directly, and it gathers them up into an {$in:[...]} > query. DataLoader is a standalone library which isn't linked to Meteor or > even to MongoDB; it just takes... something, and a callback to request all > the somethings at once. > > At the start of each request, Apollo makes a DataLoader for each > collection, which groups together lookup-object-by-ID queries. This is > great if what you have is IDs, but useless if what you have is some other > batchable query, like the cases of allVotes and currentUserVote. > > — > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > <#765 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AIq25ZXU7grexzGP7HeZ42rYTgdumkzvks5uLoGrgaJpZM4Vlsfv> > . >

jimrandomh · 2018-07-30T15:06:58Z

Note that I'm using "join" in the broad, conceptual sense of the word, not the literal sense; since MongoDB does not support joins, nothing we do is ever literally a join. In that broad sense of the word, any solution to this problem must be a join, since the problem to be solved is "get the current user's vote for each comment".

Couldn't we just do a separate non-blocking query for all the votes, and then enhance the store on the client side as soon as that one completes? That seems like the correct way to do it in terms of querying hierarchy, and would allow us to make use of all the standard infrastructure that Vulcan has without needing to change stuff super much. I guess we would have to write some logic to enhance the store, which might turn out to be similarly complicated, but it does feel more isolated than trying to do fancy stuff with the resolvers.

My current sense is that doing it that way is much more complicated, to the point where I'm not sure it's even viable. There are a lot of complicating factors:

Comments have multiple parent contexts: the Recent Discussion display on the front page, user profile pages, and post previews, for example, each of which would need to handle collecting and providing votes.
Identifying the set of comments that votes needed to be loaded for is complicated, especially in contexts where pagination can't be ignored (eg Recent Discussion).
We might later need currentUserVotes on entities which are not comments, such as posts on the front page or a Sequences page.
It isn't acceptable from a performance perspective to rerender all the comments on a post page when one comment is voted on (the vote button needs to rerender with the update in ~100ms).

Looking at older versions of Vulcan, it looks like the performance problems were introduced in this commit; before then, each post/comment had an upvoters and downvoters field directly on it, so there was no need to make a separate query to find out whether the current user had voted. That has its own set of scalability problems, but the constant factors were good enough to be viable.

jimrandomh · 2018-07-30T15:13:17Z

We could also use a virtual DOM to cut the number of comments loaded from 500 down to a dozen, which would also solve the performance problem. But that has some pretty significant disadvantages: it breaks Ctrl+F, it interacts badly with anchors (worst case: an anchor to a comment near the bottom, in which case we're back where we started), and we can't do it to GoogleBot.

jimrandomh · 2018-07-30T15:20:04Z

Also, we're going to have this same issue again with subscriptions once those are moved off the user object. And once we transition from worrying about minute-long loads to worrying about hundreds of milliseconds, we're going to start seeing this issue everywhere, and we'll need a general solution.

Discordius · 2018-07-30T17:57:03Z

Not super related to the overall point, but Mongo does support joins!

https://docs.mongodb.com/manual/reference/operator/aggregation/lookup

Discordius · 2018-07-30T18:30:02Z

+      let query = baseQuery ? _.clone(baseQuery) : {};
+      query[groupByField] = { $in: docIDs };
+      const queryResults = await Connectors.find(collection, query);
+      const result = docIDs.map(id => _.where(queryResults, {[groupByField]: id}));


Isn't this just a group by? https://underscorejs.org/#groupBy

It is. I adapted this from Vulcan's findByIds making the smallest change that would work. I have a note reminding myself to fix both this and the original in Vulcan, to not be O(n^2).

Discordius · 2018-07-30T18:31:07Z

+  }
+  if (!collection.extraLoaders[loaderName]) {
+    collection.extraLoaders[loaderName] = new DataLoader(async docIDs => {
+      let query = baseQuery ? _.clone(baseQuery) : {};


You should set the default for the baseQuery in the construction, and then you can remove this line. I.e.

(collection, loaderName, baseQuery = {}, groupByField, id)

Discordius · 2018-07-30T18:35:34Z

+  if (!collection.extraLoaders[loaderName]) {
+    collection.extraLoaders[loaderName] = new DataLoader(async docIDs => {
+      let query = baseQuery ? _.clone(baseQuery) : {};
+      query[groupByField] = { $in: docIDs };


I think I would much prefer to do this via deconstruction, than via mutation. I.e.:

const query = { ...baseQuery, [groupByField]: { $in: docIDs } }

Yep, that's better.

Discordius · 2018-07-30T18:45:05Z

+async function getWithLoader(collection, loaderName, baseQuery, groupByField, id)
+{
+  if (!collection.extraLoaders) {
+    collection.extraLoaders = {};


Couldn't you initialize collection.extraLoaders to an empty object instead of null in the Vulcan PR, and then save yourself this whole if-block?

Yes. This happened incrementally; in an earlier incarnation it wasn't getting initialized on the Vulcan side at all (that's really more of a cache-clear than an initialization). (The preexisting by-ID dataloaders are also effectively getting cache-cleared by the preexisting line next to it.)

Discordius · 2018-07-30T18:47:23Z

Ok, after thinking about it a bit more, my model is more something:

Before I want to figure out this use-case, I want to focus on solving the more important and general problem of progressive enhancement. Because that is actually something I expect to encounter all over the place, and strikes me as the more important thing to solve systematically.

I do think we want to improve our query architecture a bunch, so this still seems good. And after thinking about it more, it's not that complicated of a solution, though I do dislike adding the state to collections, though I also can't think of a much better way. I wish we had something more local to the current query that we could attach it to (I don't like the resolvers manipulating the collection object, I would much prefer to attach it so some kind of local resolver state, especially since dataloaders are only batching and caching fields for individual queries).

jimrandomh · 2018-07-30T20:09:07Z

Oh man, my impressions of Mongo are way out of date - joins were added way back in 2015. I'll keep that in mind.

I agree regarding progressive enhancement, though it's worth noting that if we were rendering comments server side, they'd still be using this code path. Roughly speaking, my goal regarding performance is to be able to load the front page or any post page, see the stuff above the fold near-instantly (that part currently works), fling-scroll down, and not see any loading indicators or hit a false bottom (that part isn't fast enough yet).

I agree that manipulating the collection object feels like a hack, but it's at least not a new hack; Vulcan's already doing it, and we're just doing it slightly more.

Discordius · 2018-07-30T20:23:35Z

My model is that for SSR, we would just not load the votes. Which seems fine to me.

…

On Mon, Jul 30, 2018 at 1:09 PM James Babcock ***@***.***> wrote: Oh man, my impressions of Mongo are way out of date - joins were added way back in 2015. I'll keep that in mind. I agree regarding progressive enhancement, though it's worth noting that if we were rendering comments server side, they'd still be using this code path. Roughly speaking, my goal regarding performance is to be able to load the front page or any post page, see the stuff above the fold near-instantly (that part currently works), fling-scroll down, and not see any loading indicators or hit a false bottom (that part isn't fast enough yet). I agree that manipulating the collection object feels like a hack, but it's at least not a *new* hack; Vulcan's already doing it, and we're just doing it slightly more. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#765 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIq25Zo_iUgb3CS0qheCpuCGuXxE9U79ks5uL2fjgaJpZM4Vlsfv> .

jimrandomh · 2018-07-30T21:33:59Z

If it's getting re-rendered client side eventually after an initial server-side render, then yeah, leaving the votes out of the first pass is fine. (Other than the elegance issue of the two renders having to be different.)

Discordius · 2018-07-30T21:39:27Z

Yep, the JS gets rerun client side, with React intelligently trying to figure out whether any changes are necessary between the Server and Client version.

…

On Mon, Jul 30, 2018 at 2:34 PM James Babcock ***@***.***> wrote: If it's getting re-rendered client side eventually after an initial server-side render, then yeah, leaving the votes out of the first pass is fine. (Other than the elegance issue of the two renders having to be different.) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#765 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIq25eqzH2ajToUL_iWVAe3sefnJLJciks5uL3vIgaJpZM4Vlsfv> .

jimrandomh · 2018-07-31T17:45:56Z

So, while this does solve the N+1 select and does greatly improve performance, it still seems slower than it should be, so I went hunting for the next bottleneck. It turns out that, with this patch applied, loading 500 comments out of a 1k comment thread takes 2-3s for the graphql query, but clicking "load more" starts a graphql query that takes ~20s. With a 10k comment thread, loaded 500 at a time, the sequence of load times is: 3s, 21s, 46s, 90s, timeout. Interestingly, if I take out the currentUserVotes and voteCount fields, the constant factors get a lot better, but it's still definitely superlinear: 1.8s, 3.5s, 6.0s, 13s, 24s, 42s, 58s, 72s, 90s, 120s, timeout. These tests were all CPU-bound in the Meteor process, with negligible time inside MongoDB and negligible disk I/O. I also tried it with the tracing and cacheControl extensions disabled (they add a lot of bulky debug information to the graphql query responses), and that didn't help.

So, I think something in one of our frameworks is making graphql queries O(n^2) in the size of the result, independent of the number of trips to MongoDB. I'm going to try to find it.

Discordius · 2018-07-31T18:47:02Z

Importantly, we are doing a full refetch on load more, so that explains at least a constant increase. I.e. when you click load more we reload all the comments that were already loaded. We can fix this by using the 'loadMoreInc' function instead of the 'loadMore' function.

…

On Tue, Jul 31, 2018 at 10:45 AM James Babcock ***@***.***> wrote: So, while this does solve the N+1 select and does greatly improve performance, it still seems slower than it should be, so I went hunting for the next bottleneck. It turns out that, with this patch applied, loading 500 comments out of a 1k comment thread takes 2-3s for the graphql query, but clicking "load more" starts a graphql query that takes ~20s. With a 10k comment thread, loaded 500 at a time, the sequence of load times is: 3s, 21s, 46s, 90s, timeout. Interestingly, if I take out the currentUserVotes and voteCount fields, the constant factors get a lot better, but it's still definitely superlinear: 1.8s, 3.5s, 6.0s, 13s, 24s, 42s, 58s, 72s, 90s, 120s, timeout. These tests were all CPU-bound in the Meteor process, with negligible time inside MongoDB and negligible disk I/O. I also tried it with the tracing and cacheControl extensions disabled (they add a lot of bulky debug information to the graphql query responses), and that didn't help. So, I think something in one of our frameworks is making graphql queries O(n^2) in the size of the result, independent of the number of trips to MongoDB. I'm going to try to find it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#765 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIq25cLK-xhEO1ESkRkjOY1j8DweiHIKks5uMJfWgaJpZM4Vlsfv> .

jimrandomh · 2018-07-31T19:47:03Z

We were suffering from the issue described in https://sandstorm.io/news/2016-09-30-fiber-bomb-debugging-story . Basically, old versions of V8 can't handle large numbers of fibers; switching between them is O(n), so having n of them outstanding is O(n^2). The fix made it into node in nodejs/node#20727 (v10.2.0). Doing 500 queries for 500 comments exacerbated the issue greatly, because running a query involves a lot of async calls; but queries should have been much faster than they were.

The solution is to upgrade to Node 10.2.0. This is slightly more complicated than it sounds, because Meteor doesn't use the system-provided copy of node, it brings its own version (8.11), and it doesn't look like anyone has backported the fix to older versions of node.

Conversation

jimrandomh commented Jul 30, 2018

Uh oh!

Discordius commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimrandomh commented Jul 30, 2018

Uh oh!

Discordius commented Jul 30, 2018 via email

Uh oh!

Discordius commented Jul 30, 2018 via email

Uh oh!

jimrandomh commented Jul 30, 2018

Uh oh!

jimrandomh commented Jul 30, 2018

Uh oh!

jimrandomh commented Jul 30, 2018

Uh oh!

Discordius commented Jul 30, 2018

Uh oh!

Discordius Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

jimrandomh Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

Discordius Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

Discordius Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

jimrandomh Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

Discordius Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

jimrandomh Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Discordius commented Jul 30, 2018

Uh oh!

jimrandomh commented Jul 30, 2018

Uh oh!

Discordius commented Jul 30, 2018 via email

Uh oh!

jimrandomh commented Jul 30, 2018

Uh oh!

Discordius commented Jul 30, 2018 via email

Uh oh!

jimrandomh commented Jul 31, 2018

Uh oh!

Discordius commented Jul 31, 2018 via email

Uh oh!

jimrandomh commented Jul 31, 2018

Uh oh!

jimrandomh commented Aug 1, 2018

Uh oh!

Discordius commented Aug 10, 2018

Uh oh!

jimrandomh commented Aug 10, 2018

Uh oh!

jimrandomh commented Aug 10, 2018

Uh oh!

Discordius commented Aug 10, 2018

Uh oh!

jimrandomh commented Aug 10, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Discordius commented Jul 30, 2018 •

edited

Loading

jimrandomh Jul 30, 2018 •

edited

Loading