Add caching for `resolve` #3549

patrick91 · 2024-06-24T20:08:21Z

This PR adds cache to the resolve method to make sure we don't do the same work multiple times.

sourcery-ai · 2024-06-24T20:08:29Z

Reviewer's Guide by Sourcery

This pull request addresses issue #3544 by replacing the @property decorator with @cached_property for the .type method in the strawberry/arguments.py file. This change aims to improve performance by caching the result of the .type method after its first computation.

File-Level Changes

Files	Changes
`strawberry/arguments.py`	Replaced @Property with @cached_property for the .type method to cache the result and improve performance.

Tips

Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
You can change your review settings at any time by accessing your dashboard:
- Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
- Change the review language;
You can always contact us if you have any questions or feedback.

sourcery-ai

Hey @patrick91 - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 1 issue found
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

strawberry/arguments.py

codecov · 2024-06-24T20:14:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.57%. Comparing base (095463d) to head (4e72915).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3549   +/-   ##
=======================================
  Coverage   96.57%   96.57%           
=======================================
  Files         524      524           
  Lines       33629    33630    +1     
  Branches     5578     5578           
=======================================
+ Hits        32478    32479    +1     
  Misses        915      915           
  Partials      236      236

codspeed-hq · 2024-06-24T20:15:18Z

CodSpeed Performance Report

Merging #3549 will improve performances by ×7

_{Comparing fix/generic-input-perf (4e72915) with main (095463d)}

Summary

⚡ 1 improvements
✅ 12 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`fix/generic-input-perf`	Change
⚡	`test_execute_generic_input`	3,094.8 ms	439.4 ms	×7

strawberry/arguments.py

patrick91 · 2024-06-24T20:55:29Z

reminder to make a pre-release and test on strawberry django & sqlalchemy (thanks @skilkis)

erikwrede · 2024-06-24T20:57:11Z

We might be able to add additional caching here in the GraphQL core converter

patrick91 · 2024-06-24T20:58:01Z

@erikwrede yeah, but I'd like to check if we are doing too much useless work first :D

Let's see what happens if I remove the cache

patrick91 · 2024-06-25T17:38:17Z

strawberry/annotation.py

@@ -101,19 +101,17 @@ def annotation(self) -> Union[object, str]:
    def annotation(self, value: Union[object, str]) -> None:
        self.raw_annotation = value

+        self.__resolved_cached__ = None
+
    def evaluate(self) -> type:


this is used in a couple of places directly, so we might want to reintroduce the cache, but I need to double check why we use it directly

I think we only use it here:

strawberry/strawberry/types/fields/resolver.py

Line 121 in 095463d

evaled_annotation = annotation.evaluate()

@skilkis do you think it is worth caching the result here too? 😊

patrick91 · 2024-06-25T17:40:33Z

/pre-release

botberry · 2024-06-25T17:41:11Z

Pre-release

👋

Releasing commit [5224421] to PyPi as pre-release! 📦

botberry · 2024-06-25T17:41:31Z

Pre-release

👋

Pre-release 0.235.1.dev.1719337273 [5224421] has been released on PyPi! 🚀
You can try it by doing:

poetry add strawberry-graphql==0.235.1.dev.1719337273

patrick91 · 2024-06-25T18:12:52Z

just tested this on Strawberry Django and all the tests passed 😊

botberry · 2024-06-25T18:47:31Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

This release improves the performance when returning a lot of data, especially
when using generic inputs (where we got a 7x speedup in our benchmark!).

Here's the tweet text:

🆕 Release (next) is out! Thanks to @patrick91 for the PR 👏

This release improves the performance when returning a lot of data, especially
when using generic inputs (where we got a 7x speedup in our benchmark!).

Get it here 👉 https://strawberry.rocks/release/(next)

`resolve` is called every time we get the field type, which, turns out, is called a lot of times, even when creating the dataclass, maybe there's some opportunity for improvements here, but we would need to call `resolve` anyway, so it's probably not worth changing how this works

patrick91 · 2024-06-25T19:01:16Z

strawberry/annotation.py

+        return self.__resolve_cache__
+
+    def _resolve(self) -> Union[StrawberryType, type]:
+        # TODO: I wonder if this resolve should be creating types?


Leaving this here for the future 😊

`resolve` is called every time we get the field type, which, turns out, is called a lot of times, even when creating the dataclass, maybe there's some opportunity for improvements here, but we would need to call `resolve` anyway, so it's probably not worth changing how this works

Use cached property for .type

6f71bd9

sourcery-ai bot reviewed Jun 24, 2024

View reviewed changes

strawberry/arguments.py Outdated Show resolved Hide resolved

skilkis reviewed Jun 24, 2024

View reviewed changes

strawberry/arguments.py Outdated Show resolved Hide resolved

More granular cache

1c3ccf9

patrick91 force-pushed the fix/generic-input-perf branch from 75db9ca to 1c3ccf9 Compare June 25, 2024 16:46

patrick91 added 2 commits June 25, 2024 18:55

Added a comment for later

61bda5d

Remove old cache and fix a bug noted by San

fff5021

Let's see what happens if I remove the cache

patrick91 force-pushed the fix/generic-input-perf branch from 74cffa4 to fff5021 Compare June 25, 2024 17:19

Add tweet and release files

a2cb19b

patrick91 force-pushed the fix/generic-input-perf branch from b806091 to a2cb19b Compare June 25, 2024 17:35

patrick91 commented Jun 25, 2024

View reviewed changes

Fix name

5224421

botberry added bot:has-release-file bot:release-type-patch labels Jun 25, 2024

patrick91 commented Jun 25, 2024

View reviewed changes

patrick91 requested review from erikwrede and skilkis June 25, 2024 19:08

patrick91 changed the title ~~Use cached property for .type~~ Add caching for resolve Jun 25, 2024

bellini666 approved these changes Jun 25, 2024

View reviewed changes

dancoates mentioned this pull request Jun 26, 2024

Upgrade to dev version of strawberry graphql to improve performance of resolvers with generic inputs populationgenomics/metamist#841

Merged

patrick91 merged commit ea6a5c4 into main Jun 26, 2024
63 of 64 checks passed

patrick91 deleted the fix/generic-input-perf branch June 26, 2024 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add caching for `resolve` #3549

Add caching for `resolve` #3549

patrick91 commented Jun 24, 2024 •

edited

Loading

sourcery-ai bot commented Jun 24, 2024 •

edited

Loading

sourcery-ai bot left a comment

codecov bot commented Jun 24, 2024 •

edited

Loading

codspeed-hq bot commented Jun 24, 2024 •

edited

Loading

patrick91 commented Jun 24, 2024 •

edited by erikwrede

Loading

erikwrede commented Jun 24, 2024 •

edited

Loading

patrick91 commented Jun 24, 2024

patrick91 Jun 25, 2024

patrick91 Jun 25, 2024

patrick91 commented Jun 25, 2024

botberry commented Jun 25, 2024

botberry commented Jun 25, 2024

patrick91 commented Jun 25, 2024

botberry commented Jun 25, 2024 •

edited

Loading

patrick91 Jun 25, 2024

Add caching for resolve #3549

Add caching for resolve #3549

Conversation

patrick91 commented Jun 24, 2024 • edited Loading

sourcery-ai bot commented Jun 24, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

sourcery-ai bot left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Jun 24, 2024 • edited Loading

CodSpeed Performance Report

Merging #3549 will improve performances by ×7

Summary

Benchmarks breakdown

patrick91 commented Jun 24, 2024 • edited by erikwrede Loading

erikwrede commented Jun 24, 2024 • edited Loading

patrick91 commented Jun 24, 2024

patrick91 Jun 25, 2024

Choose a reason for hiding this comment

patrick91 Jun 25, 2024

Choose a reason for hiding this comment

patrick91 commented Jun 25, 2024

botberry commented Jun 25, 2024

Pre-release

botberry commented Jun 25, 2024

Pre-release

patrick91 commented Jun 25, 2024

botberry commented Jun 25, 2024 • edited Loading

patrick91 Jun 25, 2024

Choose a reason for hiding this comment

Add caching for `resolve` #3549

Add caching for `resolve` #3549

patrick91 commented Jun 24, 2024 •

edited

Loading

sourcery-ai bot commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 24, 2024 •

edited

Loading

codspeed-hq bot commented Jun 24, 2024 •

edited

Loading

patrick91 commented Jun 24, 2024 •

edited by erikwrede

Loading

erikwrede commented Jun 24, 2024 •

edited

Loading

botberry commented Jun 25, 2024 •

edited

Loading