Add unaliased path to resolve info #1548
Conversation
|
@spawnia This one is ready for review, when you have time. |
|
@spawnia You are probably busy. Do you have time for a review? Or is there anybody else that can do it? |
ruudk
force-pushed
the
unaliased-path
branch
5 times, most recently
from
659dbec to
9bd93d3
Compare
|
@spawnia this is ready now. The errors are not related to this PR. |
|
Please merge |
|
It's green now. |
Do they succeed after a while or do they loop forever? |
|
Seems like loop forever. I've waited 5m and nothing got finished. While on 15.11 it's done in ~10s (larger query). |
|
Do you have a tip or should I start profilling? |
|
Interesting. We run a very large GraphQL schema in production with a lot of traffic and did not notice anything different. |
|
My other app works fine. Also I cannot reproduce locally. I synced php patch versions and still the issue is only in prod env, not local one. I'm trying to investigate further. |
|
I have compared the two versions, you can see where the latest one is slower https://blackfire.io/profiles/compare/07b25a5c-c984-49fb-a134-5b4026020104/graph |
|
What kind of query did you use? Is this a big query that is using a big list or is it nested deep? While looking at the diff in |
| { | ||
| $containsPromise = false; | ||
| $results = []; | ||
| foreach ($fields as $responseName => $fieldNodes) { | ||
| $fieldNodes = $fields[$responseName]; |
There was a problem hiding this comment.
@simPod This might be the issue. This line does not make any sense, $fieldNodes is already defined in the foreach.
There was a problem hiding this comment.
@simPod Can you check if https://github.com/webonyx/graphql-php/releases/tag/v15.12.1 fixes your issue?
Somehow this was added in webonyx#1548 but I fail to see why. Let's remove it. This might be the cause for the performance degradation reported in webonyx#1548.
Somehow this was added in webonyx#1548 but I fail to see why. Let's remove it. This might be the cause for the performance degradation reported in webonyx#1548. Add property `unaliasedPath` to `ResolveInfo`
- make some lines a bit shorter Still did not find a way to avoid performance slowdown introduced in webonyx#1548. This is just an outcome of few hours I looked at the code.
@ruudk the issue appears when you return a lot of entries. For each entry it computes the path and now unaliasedPath. That causes the slowdown. The query depth is not relevant. If you're returning small datasets you won't notice the slowdown. |
|
I think it would be great to have a unit test that reproduces the issue. Then we can work on making this faster. Could you create it ? |
Fixes #1345