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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overall improvement of the storefront 2.0 performances #3968

Merged

Conversation

NyanKiyoshi
Copy link
Member

@NyanKiyoshi NyanKiyoshi commented Apr 15, 2019

This fixes #3631.

Changes

Optimization of Single Nodes

This pull enables optimizations for single nodes. Which optimizes a lot of requests. See tfoxy/graphene-django-optimizer#18 for more information about this.

Overriding the Retrieval of a Single Node

Following the tfoxy/graphene-django-optimizer#18 changes, getting a single node whether optimized or not is now simpler. You should always call maybe_optimize(info, qs, pk) and not directly call get_optimized_node(info, qs, pk) when overriding get_node(info, pk) as it would result to issues when the node is not supposed to actually get optimized.

This sums up of doing this:

Screenshot 2019-04-18 at 09 54 42

During The Payment Capture

  • Prefetch order lines that then get proceeded;
  • Prefetch the user that will later get used to get the email address.

Comparison

Page Before After
Product Listing 29 queries 11 queries
Product Details 40 queries 14 queries
Checkout Shipping Method Update 82 queries 38 queries
Placing an Order 135 queries 64 queries
... ... ...

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. Database migration files are up to date.
  6. The changes are tested.
  7. The code is documented (docstrings, project documentation).
  8. GraphQL schema and type definitions are up to date.
  9. Changes are mentioned in the changelog.

@NyanKiyoshi NyanKiyoshi added in progress performance graphql Issues related to the GraphQL API labels Apr 15, 2019
@NyanKiyoshi NyanKiyoshi added this to the Sprint 8 - Bulk actions milestone Apr 15, 2019
@NyanKiyoshi NyanKiyoshi self-assigned this Apr 15, 2019
@NyanKiyoshi NyanKiyoshi force-pushed the optimize-queries/storefront-2.0 branch from 2502725 to 4bb1c4d Compare April 15, 2019 15:16
@NyanKiyoshi NyanKiyoshi changed the title Overall improvement of the storefront 2.0 queries Overall improvement of the storefront 2.0 performances Apr 15, 2019
@NyanKiyoshi
Copy link
Member Author

There are bugs/issues around the optimizer library, so this PR will sadly (brings a lot of great optimizations) not be ready until the root of this issue is figured out. Hopefully I can dig more tomorrow around the troubles (relates to saleor/saleor-storefront#295).

@NyanKiyoshi NyanKiyoshi modified the milestones: Sprint 8 - Bulk actions, imirumee, Sprint 9 Apr 16, 2019
@maarcingebala maarcingebala removed this from the Sprint 9 milestone Apr 17, 2019
@NyanKiyoshi NyanKiyoshi force-pushed the optimize-queries/storefront-2.0 branch 2 times, most recently from 8c3f6f1 to 83d2b00 Compare April 17, 2019 16:39
@NyanKiyoshi
Copy link
Member Author

@auvipy please don't review drafts, that generating useless notifications on my side (thus making me lose precious time). Just leave a little 👍 reaction on the PR if you like it, that's enough to say you approve it!

@NyanKiyoshi NyanKiyoshi force-pushed the optimize-queries/storefront-2.0 branch from 65be03c to 810839a Compare April 18, 2019 07:55
@NyanKiyoshi
Copy link
Member Author

NyanKiyoshi commented Apr 18, 2019

I added from_global_id_strict_type that has the aim to get easily a node from a given ID while using a custom queryset instead of using a simple .get(pk) that doesn't prefetch anything.

Let me know what you think, or if you see a better approach to this matter.

@NyanKiyoshi NyanKiyoshi force-pushed the optimize-queries/storefront-2.0 branch from 1cf1a18 to ace5fc9 Compare April 30, 2019 03:52
@NyanKiyoshi NyanKiyoshi marked this pull request as ready for review April 30, 2019 03:53
@codecov
Copy link

codecov bot commented Apr 30, 2019

Codecov Report

Merging #3968 into master will decrease coverage by 0.05%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3968      +/-   ##
==========================================
- Coverage   91.37%   91.32%   -0.06%     
==========================================
  Files         274      274              
  Lines       14947    14950       +3     
  Branches     1451     1452       +1     
==========================================
- Hits        13658    13653       -5     
- Misses        898      904       +6     
- Partials      391      393       +2
Impacted Files Coverage Δ
saleor/graphql/payment/mutations.py 97.7% <100%> (+0.02%) ⬆️
saleor/graphql/checkout/mutations.py 93.47% <100%> (+0.04%) ⬆️
saleor/graphql/product/types/products.py 93.1% <100%> (-2.19%) ⬇️
saleor/dashboard/order/views.py 79.25% <100%> (ø) ⬆️
saleor/graphql/core/connection.py 100% <100%> (ø) ⬆️
saleor/graphql/core/utils.py 91.3% <71.42%> (-8.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b83a0eb...70e7aa5. Read the comment docs.

Copy link
Member

@maarcingebala maarcingebala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good, but I'm only wondering about the from_global_id_strict_type function. I'm not sure if this should be a part of BaseMutation since it's only used in two places and is meant rather for specific use cases. I guess it should be a utility function somewhere in "saleor/graphql/core".

@maarcingebala maarcingebala merged commit 615ba08 into saleor:master Apr 30, 2019
@NyanKiyoshi NyanKiyoshi deleted the optimize-queries/storefront-2.0 branch April 30, 2019 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graphql Issues related to the GraphQL API performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize Storefront 2.0 queries and mutations
3 participants