Improve perf of separateOperations #710
Conversation
src/utilities/separateOperations.js
Outdated
| }, | ||
| FragmentDefinition(node) { | ||
| fromName = node.name.value; | ||
| definitions[fromName] = { idx, node }; |
There was a problem hiding this comment.
There's currently nothing to guarantee that an operation and fragment definition will always have distinct names. It would be safer to have separate operationDefinitions and fragmentDefinitions maps
There was a problem hiding this comment.
Also - lookups into this map are happening only for fragments, so you only need these
There was a problem hiding this comment.
Oh I didn't realize definition names were not global.
|
Awesome! I'll do a more careful review but I'm curious if you were able to collect any performance numbers before and after this change? |
src/utilities/separateOperations.js
Outdated
| @@ -47,23 +52,32 @@ export function separateOperations( | |||
| // For each operation, produce a new synthesized AST which includes only what | |||
| // is necessary for completing that operation. | |||
| const separatedDocumentASTs = Object.create(null); | |||
| operations.forEach(operation => { | |||
| const operationName = opName(operation); | |||
| const operationNames = Object.keys(definitions).filter(defName => | |||
There was a problem hiding this comment.
Leaving the existing list of operations as it was would remove the need to do a second pass through all the definitions
|
The change in performance was significant. I was testing on a document with 1796 operations and 3818 fragments. Before separateOperations took 2.8 seconds. Afterward it took 0.4 seconds. That inner loop of 5000+ iterations dropped to about 60 iterations on average (# of transitive fragments per query in my doc). |
|
7x improvement is nothing to sneeze at! Nice work. I just pushed on an alteration that uses |
|
Awesome! |
|
Will this be in |
The separateOperations function does a pass over all definitions of a document for each operation in the document. This is painful for big documents. Since the function already visits each definition node, we can cache the visited nodes. Then instead of passing over all top-level definitions in the document and removing those not in an operation's depgraph, we can pass over the (often much smaller) list of definition names in the depgraph and retrieve the definitions by name from the cache.
Ran flow check and separateOperations-test.js