Improve performance of batched requests - #64266
Wesley Wigham (weswigham) wants to merge 5 commits into
Conversation
Daniel Rosenwasser (DanielRosenwasser)
left a comment
There was a problem hiding this comment.
First take - I'm not 100% certain.
Currently this ends up doing a lot of allocation of arrays and closures, though we could maybe optimize that.
But there's still some preprocessing. Maybe that's okay for if you use the batched API in general, but if other APIs internally use the batch API they have to pay for this.
Also, originally I was thinking that this would live entirely in the JS library, but it looks like the server itself has to understand this grouping notion to support batching.
I'm not against this, but wondering what others think and might want to sit with this to think a bit.
| requestsByMethod[groupIndex].push(request); | ||
| groupOrder.push(groupIndex); | ||
| } | ||
| if (!requestsByMethod.some(group => group.length >= 4)) return undefined; |
There was a problem hiding this comment.
I think avoiding a double-walk/allocating the closure is worth inlining into the body.
There was a problem hiding this comment.
Ofc, moved it in and shared even more work for (some) similar requests within a batch. I'm at least pretty optimistic that despite removing the wire protocol for the stuff like the array overload of getSymbolAtLocation, using getSymbolAtLocation with an array argument in the client should be just as performant as before, and batching a bunch of sibling calls to imitate that should be much closer.
A lot of the logic is definitely on the JS side, yeah, and to start I did just make the JS side automagically defer to the plural APIs when a batch contained multiple requests to the same singular one, but keeping it like that would have entailed a lot of future work to maintain two wire protocol implementations for each protocol method we valued bulk operation performance of (one bulk, one singular). I didn't want to do that or force us to maintain that (keeping non-codegen'd things in sync is annoying). So, instead, I updated the server to support the same kind of efficient many-calls-with-shared-parameters that the manual |
So, full compression was a wash, but making the client group requests automatically to save on both wire traffic and checker-lock-acquisitions on the backend was useful. I've removed all the hardcoded plural batch entrypoints from the wire protocol here because the general batch mechanisms can accomplish the same performance now, but for every API endpoint, with the help of a little codegen. The clientside overloads are still present for convenience, though.
cc Daniel Rosenwasser (@DanielRosenwasser) Yeah, you were right, we basically had to swap from array of structs to struct of arrays to have better allocation performance. While this looks good on the microbenchmarks, it'd be nice if you could see if this improves perf in your example, too, before we go adding this complexity.