Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve graphql perf #4158

Merged
merged 4 commits into from Oct 4, 2019

Conversation

@alexandrebodin
Copy link
Collaborator

alexandrebodin commented Oct 1, 2019

Description of what you did:

Might Fix #4080 it needs testing by @JelmerV-WFC

My PR is a:

  • 馃挜 Breaking change
  • 馃悰 Bug fix
  • 馃拝 Enhancement
  • 馃殌 New feature

Main update on the:

  • Admin
  • Documentation
  • Framework
  • Plugin

Manual testing done on the following databases:

  • Not applicable
  • MongoDB
  • MySQL
  • Postgres
  • SQLite
@alexandrebodin alexandrebodin requested a review from derrickmehaffy Oct 1, 2019
@derrickmehaffy

This comment has been minimized.

Copy link
Contributor

derrickmehaffy commented Oct 1, 2019

Ty @alexandrebodin I'll get to testing this

@JelmerV-WFC

This comment has been minimized.

Copy link
Contributor

JelmerV-WFC commented Oct 1, 2019

Will test it, thanks @alexandrebodin!

@JelmerV-WFC

This comment has been minimized.

Copy link
Contributor

JelmerV-WFC commented Oct 1, 2019

I don't think it's improving much at my side, at first sight. But it can be my connection that's the issue right now (on 4G in a train ;-)).

@JelmerV-WFC

This comment has been minimized.

Copy link
Contributor

JelmerV-WFC commented Oct 2, 2019

@alexandrebodin is it possible that after this merging this pull request the relations are not returned anymore? All relations are returning null.

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator Author

alexandrebodin commented Oct 2, 2019

@JelmerV-WFC yes the PR is only working for SQL a the moment if you are using mongo I need to do some updates. Will keep you updated

@JelmerV-WFC

This comment has been minimized.

Copy link
Contributor

JelmerV-WFC commented Oct 2, 2019

@JelmerV-WFC yes the PR is only working for SQL a the moment if you are using mongo I need to do some updates. Will keep you updated

Okay! Thanks!

@alexandrebodin alexandrebodin changed the title Improve graphql perf [WIP] Improve graphql perf Oct 2, 2019
@alexandrebodin alexandrebodin changed the title [WIP] Improve graphql perf Improve graphql perf Oct 2, 2019
@alexandrebodin

This comment has been minimized.

Copy link
Collaborator Author

alexandrebodin commented Oct 2, 2019

@JelmerV-WFC Can you test this again ?

@JelmerV-WFC

This comment has been minimized.

Copy link
Contributor

JelmerV-WFC commented Oct 2, 2019

@alexandrebodin it did improve the performance for what I can see. @derrickmehaffy did you had the time to test it?

@derrickmehaffy

This comment has been minimized.

Copy link
Contributor

derrickmehaffy commented Oct 2, 2019

Not yet @JelmerV-WFC I'll be doing it tonight when I get to work.

Copy link
Contributor

derrickmehaffy left a comment

Oh dude @alexandrebodin This is a night and day difference man.

Pre changes (beta16.7):

[2019-10-03T05:43:51.711Z] debug POST /graphql?_limit=1000&verified=false (2494 ms) 200
[2019-10-03T05:44:18.294Z] debug POST /graphql?_limit=1000&verified=false (1355 ms) 200

Post PR changes:

[2019-10-03T05:46:24.425Z] debug POST /graphql?_limit=1000&verified=false (373 ms) 200
[2019-10-03T05:46:28.094Z] debug POST /graphql?_limit=1000&verified=false (288 ms) 200
[2019-10-03T05:46:35.812Z] debug POST /graphql?_limit=1000&verified=false (128 ms) 200

Nice job LFGTM 馃憤

@JelmerV-WFC

This comment has been minimized.

Copy link
Contributor

JelmerV-WFC commented Oct 3, 2019

I also can confirm that this fixes the performance issues with Mongoose!

@alexandrebodin alexandrebodin merged commit 5bc4a0b into master Oct 4, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@alexandrebodin alexandrebodin deleted the fix/improve-graphql-perf branch Oct 4, 2019
@devyourcar

This comment has been minimized.

Copy link

devyourcar commented Oct 6, 2019

@alexandrebodin thanks so much for this, the responses are feeling snappier on our prod env!

@mbistuer

This comment has been minimized.

Copy link

mbistuer commented Oct 8, 2019

@alexandrebodin
hello, it works fine with custom types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.