polish(fragmentArgs): simplify fragment args execution #4182
Conversation
|
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Closing for now as I think I like what we have currently better, but anyone can feel free to chime in and we can reopen as necessary. |
|
Reopening as I got some positive feedback offline for this approach. This also helps with a bug I believe I noticed, in that we did not remember to pass fragmentVariables to parseLiteral for scalars, have to investigate further, but this PR could be a solution as well. |
by creating a map for each fragment with fragment variables with the properly combined local and global variables background: this was in the original PR graphql#4015, was taken out by me when i misunderstood and thought that when a fragment variable shadows an operation variable, the operation variable could still leak through if the fragment variable had no default, and I thought it was necessary within getArgumentValues to still have the signatures advantages to the original approach, which this PR restores: does not change the signature of getVariableValues, getDirectiveValues, valueFromAST disadvantages: creating a combined variable map of local and global variables may be a waste if the global variables are not actually used in that fragment
yaacovCR
force-pushed
the
simplify-fragment-args-execution
branch
from
aaaf061
to
884f67c
Compare
yaacovCR
added
the
PR: polish 💅
|
So with this new approach of coalescing the fragmentVariableValues and operationVariableValues, we will have to perform a copy operation for each fragment spread with arguments, for each operation variable value, for a total of FragmentSpreadWithArguments * OperationVariableValues copy operations. With the existing approach, we have to perform an additional check * the number of VariableUsages. As the number of fragment spreads with arguments and operation variable values increases, the existing approach is faster. I added a benchmark with a 100 of each, and I get the following:
-- This PR gives about a 20% speed drop, and a 25% memory increase. When resetting the benchmark to 20 of each, I get the following:
-- This PR gives about a 6% speed drop, and an 8% memory increase. When setting to 1 of each, I get:
-- This PR gives a 2% speed increase and a less than 1% memory increase. I think these results basically work with the big O analysis above => and to me they suggest we stick with the original version, but, of course, the speed/memory tradeoffs have to be balanced against the overall change to the API. It might make sense to keep this PR in the experimental stage and then re-optimize later. |
|
Closing for now, nice to have this as a foil/strawman. |
...by creating a map for each fragment with fragment variables with the combined local and global variables
background: this was in the original version PR #4015, was taken out -- by me -- when i misunderstood and thought that when a fragment variable shadows an operation variable, the operation variable could still leak through if the fragment variable had no default, and I thought it was necessary within getArgumentValues to still have the signatures. that was fixed, but the original method of coalescing local and global variables within a map was not restored, in favor of just retaining both maps
advantage to the original approach, which this PR restores => does not change the signature of getVariableValues, getDirectiveValues, valueFromAST. the lazy method retains both maps and requires passing the local fragment variables to those functions
disadvantage to the original approach => creating a combined variable map of local and global variables may be a waste if the global variables are not actually used in that fragment
I am not sure which way is better! Just raising this for discussion.