Make gql js faster #4305
Make gql js faster #4305
Conversation
|
Hi @JoviDeCroock, 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:
|
JoviDeCroock
force-pushed
the
make-gql-js-faster
branch
from
71a9fe8
to
d492cb1
Compare
JoviDeCroock
force-pushed
the
make-gql-js-faster
branch
from
d492cb1
to
2d0415d
Compare
This comment has been minimized.
This comment has been minimized.
Benchmark output
🍳 Preparing HEAD... HEAD�[90m x �[0m�[32m45.51�[0m ops/sec �[90m±�[0m�[33m2.21�[0m�[36m%�[0m�[90m x �[0m2.18 MB/op�[90m (8 runs sampled)�[0m ⏱ Build Schema from Introspection �[32mHEAD�[0m�[90m x �[0m�[32m54.13�[0m ops/sec �[90m±�[0m�[32m0.67�[0m�[36m%�[0m�[90m x �[0m1.11 MB/op�[90m (11 runs sampled)�[0m ⏱ Execute Introspection Query �[32mHEAD�[0m�[90m x �[0m�[32m49.44�[0m ops/sec �[90m±�[0m�[33m2.37�[0m�[36m%�[0m�[90m x �[0m2.48 MB/op�[90m (8 runs sampled)�[0m ⏱ Parse introspection query HEAD�[90m x �[0m�[32m13,613�[0m ops/sec �[90m±�[0m�[32m0.16�[0m�[36m%�[0m�[90m x �[0m2.62 KB/op�[90m (35 runs sampled)�[0m ⏱ Print ktichen-sink query �[32mHEAD�[0m�[90m x �[0m�[32m2,696�[0m ops/sec �[90m±�[0m�[32m1.10�[0m�[36m%�[0m�[90m x �[0m875 Bytes/op�[90m (11 runs sampled)�[0m ⏱ Many repeated fields HEAD�[90m x �[0m�[32m 99.1�[0m ops/sec �[90m±�[0m�[32m0.38�[0m�[36m%�[0m�[90m x �[0m524 KB/op�[90m (20 runs sampled)�[0m ⏱ Validate Introspection Query �[32mHEAD�[0m�[90m x �[0m�[32m571�[0m ops/sec �[90m±�[0m�[32m0.33�[0m�[36m%�[0m�[90m x �[0m268 KB/op�[90m (14 runs sampled)�[0m ⏱ Validate Invalid Query �[32mHEAD�[0m�[90m x �[0m�[32m509�[0m ops/sec �[90m±�[0m�[32m0.19�[0m�[36m%�[0m�[90m x �[0m326 KB/op�[90m (14 runs sampled)�[0m ⏱ Validate SDL Document HEAD�[90m x �[0m�[32m67.11�[0m ops/sec �[90m±�[0m�[32m1.26�[0m�[36m%�[0m�[90m x �[0m221 KB/op�[90m (11 runs sampled)�[0m ⏱ Visit all AST nodes HEAD�[90m x �[0m�[32m302�[0m ops/sec �[90m±�[0m�[32m0.15�[0m�[36m%�[0m�[90m x �[0m310 KB/op�[90m (20 runs sampled)�[0m ⏱ Visit all AST nodes in parallel �[32mHEAD�[0m�[90m x �[0m�[32m28.95�[0m ops/sec �[90m±�[0m�[32m0.53�[0m�[36m%�[0m�[90m x �[0m1.23 MB/op�[90m (7 runs sampled)�[0m |
This comment has been minimized.
This comment has been minimized.
| @@ -222,10 +224,7 @@ export function visit( | |||
| } | |||
| } | |||
| } else { | |||
| node = Object.defineProperties( | |||
There was a problem hiding this comment.
This is a massive deopt, maybe we could keep this in by i.e. rather than copying the node just returning the string if that's what the edit is symbolising
Benchmark output
🍳 Preparing HEAD... �[32mHEAD�[0m�[90m x �[0m�[32m44.56�[0m ops/sec �[90m±�[0m�[32m1.54�[0m�[36m%�[0m�[90m x �[0m2.18 MB/op�[90m (8 runs sampled)�[0m ⏱ Build Schema from Introspection HEAD�[90m x �[0m�[32m51.34�[0m ops/sec �[90m±�[0m�[32m1.30�[0m�[36m%�[0m�[90m x �[0m1.11 MB/op�[90m (11 runs sampled)�[0m ⏱ Execute Introspection Query HEAD�[90m x �[0m�[32m49.17�[0m ops/sec �[90m±�[0m�[33m3.65�[0m�[36m%�[0m�[90m x �[0m2.48 MB/op�[90m (8 runs sampled)�[0m ⏱ Parse introspection query HEAD�[90m x �[0m�[32m13,239�[0m ops/sec �[90m±�[0m�[33m2.48�[0m�[36m%�[0m�[90m x �[0m2.62 KB/op�[90m (33 runs sampled)�[0m ⏱ Print kitchen-sink query �[32mHEAD�[0m�[90m x �[0m�[32m9,620�[0m ops/sec �[90m±�[0m�[32m0.28�[0m�[36m%�[0m�[90m x �[0m2.14 KB/op�[90m (25 runs sampled)�[0m ⏱ Many repeated fields �[32mHEAD�[0m�[90m x �[0m�[32m99.53�[0m ops/sec �[90m±�[0m�[32m0.35�[0m�[36m%�[0m�[90m x �[0m524 KB/op�[90m (21 runs sampled)�[0m ⏱ Validate Introspection Query HEAD�[90m x �[0m�[32m553�[0m ops/sec �[90m±�[0m�[33m3.91�[0m�[36m%�[0m�[90m x �[0m268 KB/op�[90m (14 runs sampled)�[0m ⏱ Validate Invalid Query �[32mHEAD�[0m�[90m x �[0m�[32m502�[0m ops/sec �[90m±�[0m�[32m0.70�[0m�[36m%�[0m�[90m x �[0m326 KB/op�[90m (14 runs sampled)�[0m ⏱ Validate SDL Document HEAD�[90m x �[0m�[33m61.47�[0m ops/sec �[90m±�[0m�[33m2.66�[0m�[36m%�[0m�[90m x �[0m221 KB/op�[90m (10 runs sampled)�[0m ⏱ Visit all AST nodes HEAD�[90m x �[0m�[33m290�[0m ops/sec �[90m±�[0m�[33m4.58�[0m�[36m%�[0m�[90m x �[0m310 KB/op�[90m (19 runs sampled)�[0m ⏱ Visit all AST nodes in parallel HEAD�[90m x �[0m�[32m28.49�[0m ops/sec �[90m±�[0m�[32m1.99�[0m�[36m%�[0m�[90m x �[0m1.23 MB/op�[90m (7 runs sampled)�[0m |
Might want to make the PR title a bit more specific tho :) |
|
After evaluation it doesn't look safe to remove property-descriptors 😅 this would be a breaking change |
While researching the numbers on v15 vs v16 I realised that our printer became much slower, the reason for this is two-fold.
joinfunction thatfilterseven when it's not neededvisitwe always recreate the full treeNow that second point we should discuss, as changing this might be considered a breaking change.
The way
visitcurrently behaves is that it is a reducer, whether or not the user returns a value it will always fully recreate the AST without any attempt at reusing parts of it, or as is the issue with our printer, it will recreate the full AST even if we aren't even remotely returning anything AST-like, we are returning strings 😅This PR can serve as an exploration of how we can improve things in v17 - I've seen issues flying around about refactoring the visitor, which this seems like a good trigger judging from how the performance is.