-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[intepreter][Canvas] Dedupe server functions in batched requests #32712
[intepreter][Canvas] Dedupe server functions in batched requests #32712
Conversation
Pinging @elastic/kibana-canvas |
💔 Build Failed |
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.
Seems right. Adding some tests would certainly be nice. Curious if you're seeing much of a performance gain here... it will probably help with the network side of things even if not.
|
||
// Check to see if this is a duplicate server function. | ||
const duplicate = Object.values(batch).find(batchedRequest => | ||
_.isMatch(batchedRequest.request, request) |
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.
Looks like you're using isMatch
here instead of isEqual
because you're trying to ignore the id
param. Is that right?
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.
Yes, that's right.
@w33ble Here's the difference with YDYW: about 600k savings overall. Seems like a good fix... it would be amazing if we could batch that across requests. Had we not decided to start loading pages separately, we probably could have done that. I'll add some tests. |
You might be able to find a sweet spot by tweaking that timeout in the batched fetch code. 10ms is pretty small, maybe something like 25, 50, or 100 would let you de-dupe more without a noticable lag. It still wouldn't help across multiple pages, but it may speed up inital loading for each one. Just a guess though. |
💚 Build Succeeded |
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.
Neat change, LGTM!
…stic#32712) * [intepreter][Canvas] Dedupe server functions in batched requests * Add and correct tests
…stic#32712) * [intepreter][Canvas] Dedupe server functions in batched requests * Add and correct tests
…stic#32712) * [intepreter][Canvas] Dedupe server functions in batched requests * Add and correct tests
#32712) (#32833) * [intepreter][Canvas] Dedupe server functions in batched requests (#32712) * [intepreter][Canvas] Dedupe server functions in batched requests * Add and correct tests * Update interpreter.test.js Fix merge error * Delete batched_fetch.test.js This file was not present in the branch and is failing.
) (#32830) * [intepreter][Canvas] Dedupe server functions in batched requests * Add and correct tests
…stic#32712) * [intepreter][Canvas] Dedupe server functions in batched requests * Add and correct tests
…stic#32712) * [intepreter][Canvas] Dedupe server functions in batched requests * Add and correct tests
…stic#32712) * [intepreter][Canvas] Dedupe server functions in batched requests * Add and correct tests
Summary
Currently, if a set of batched function calls are identical, they are all called. For example, if a page calls for
demodata
in a number of elements,demodata
is requested and returned for each element, (ballooning the size of the response).This PR dedupes the server calls, greatly reducing the payload size.
Before:
After: