Improve performance of `convert_argument` for list of primitives #3773

blothmann · 2025-02-10T09:55:25Z

Improve performance of convert_argument for list types that do not need any conversion of its arguments by just returning a copy of the list.

Description

This has a drastical improvement of the performance if the input list is large. In my use case under some circumstances I am handling lists of hundred thousands of elements. Instead of iterating through each element, checking some conditions and returning the element, just return a copy of the list is a huge performance improvement.

Since tests cover everything that I have changed already, I added a performance test for large lists of integers.

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Summary by Sourcery

Optimize argument conversion for lists of primitive types by directly returning a copy of the list if no type conversion is required.

Enhancements:

Improve performance of convert_argument for large lists of primitive types.

Tests:

Add performance tests for converting large lists of integers.

…lars, optional scalars, enums)

sourcery-ai · 2025-02-10T09:55:31Z

Reviewer's Guide by Sourcery

This pull request improves the performance of the convert_argument function for lists of primitive types by adding an early return mechanism and avoiding unnecessary element-by-element processing. The changes include a new helper function to determine if conversion can be bypassed, adjustments in the convert_argument logic for list types, and additional performance benchmarks.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change	Details	Files
Improved argument conversion logic by adding an early return for list types containing primitives.	Introduced a new helper function that checks if the type of argument requires conversion. Modified the convert_argument function to immediately return the input value if conversion is unnecessary. Optimized handling of StrawberryList by returning a shallow copy when its inner type doesn't need conversion.	`strawberry/types/arguments.py`
Added performance benchmarks for large list arguments.	Created a new test to benchmark the convert_argument function using large lists of integers.	`tests/benchmarks/test_arguments.py`
Updated release documentation to reflect performance improvements.	Added a patch release note emphasizing improved performance for argument conversion.	`RELEASE.md`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!
Generate a plan of action for an issue: Comment @sourcery-ai plan on
an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

for more information, see https://pre-commit.ci

sourcery-ai

Hey @blothmann - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider adding a comment to _convert_argument_value_can_be_returned explaining why LazyType and enums need to be resolved.
It might be worth adding a comment explaining why we can return the original value for scalars and enums.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟡 Testing: 1 issue found
🟢 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 and I'll use the feedback to improve your reviews.}

tests/benchmarks/test_arguments.py

botberry · 2025-02-10T09:56:45Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

This release improves performance of argument conversion for lists of primitives.

Here's the tweet text:

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

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

patrick91 · 2025-02-10T10:11:09Z

strawberry/types/arguments.py

@@ -135,6 +135,31 @@ def is_graphql_generic(self) -> bool:
        return is_graphql_generic(self.type)


+def _convert_argument_value_can_be_returned(


can we find a different name for this? :D I don't think it is easy to understand (maybe we can add a doc string too)?

Great stuff btw 😊

We are looking for a scalar or enum or an optional version of those, which can simply be returned in convert_argument. So maybe something like _is_primitive?

codecov · 2025-02-10T10:14:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.23%. Comparing base (2d4234a) to head (6e8cc84).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3773      +/-   ##
==========================================
- Coverage   97.24%   97.23%   -0.02%     
==========================================
  Files         504      503       -1     
  Lines       33480    33487       +7     
  Branches     5502     1707    -3795     
==========================================
+ Hits        32558    32561       +3     
- Misses        703      708       +5     
+ Partials      219      218       -1

codspeed-hq · 2025-02-10T10:22:33Z

CodSpeed Performance Report

Merging #3773 will not alter performance

_{Comparing blothmann:optimize-convert-argument-list (6e8cc84) with main (2af3743)}

Summary

✅ 21 untouched benchmarks
🆕 5 new benchmarks

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
🆕	`test_convert_argument_large_list[1048576]`	N/A	78 ms	N/A
🆕	`test_convert_argument_large_list[16384]`	N/A	783.4 µs	N/A
🆕	`test_convert_argument_large_list[262144]`	N/A	16.8 ms	N/A
🆕	`test_convert_argument_large_list[4194304]`	N/A	313.2 ms	N/A
🆕	`test_convert_argument_large_list[65536]`	N/A	2.9 ms	N/A

blothmann · 2025-02-11T10:18:32Z

Tried to make the function naming more clear to determine in which case a copy of the list can be returned: if item type is a leaf (_is_leaf_type) or an optional leaf (_is_optional_leaf_type). Same naming (is_leaf_type) is also used in graphql-core library.
Added test case for optional leaf elements.

Improve performance of convert_argument for list of primitives (sca…

50dfa25

…lars, optional scalars, enums)

[pre-commit.ci] auto fixes from pre-commit.com hooks

6e8cc84

for more information, see https://pre-commit.ci

sourcery-ai bot reviewed Feb 10, 2025

View reviewed changes

tests/benchmarks/test_arguments.py Show resolved Hide resolved

botberry added bot:has-release-file bot:release-type-patch labels Feb 10, 2025

patrick91 reviewed Feb 10, 2025

View reviewed changes

blothmann added 4 commits February 10, 2025 13:09

refactor bad naming

924e95f

refactor to _is_leaf_type, which is compatible to graphql-core wording

572b85f

refactor _is_optional_leaf_type to prevent nested type_.of_type.of_type

f6bec1b

Enhance test for optional elements in lists.

ff33cc8

blothmann requested a review from patrick91 February 11, 2025 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of `convert_argument` for list of primitives #3773

Improve performance of `convert_argument` for list of primitives #3773

blothmann commented Feb 10, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Feb 10, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

botberry commented Feb 10, 2025 •

edited

Loading

patrick91 Feb 10, 2025

blothmann Feb 10, 2025

codecov bot commented Feb 10, 2025 •

edited

Loading

codspeed-hq bot commented Feb 10, 2025

blothmann commented Feb 11, 2025

		@@ -135,6 +135,31 @@ def is_graphql_generic(self) -> bool:
		return is_graphql_generic(self.type)


		def _convert_argument_value_can_be_returned(

Improve performance of convert_argument for list of primitives #3773

Are you sure you want to change the base?

Improve performance of convert_argument for list of primitives #3773

Conversation

blothmann commented Feb 10, 2025 • edited by sourcery-ai bot Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Summary by Sourcery

sourcery-ai bot commented Feb 10, 2025 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

botberry commented Feb 10, 2025 • edited Loading

patrick91 Feb 10, 2025

Choose a reason for hiding this comment

blothmann Feb 10, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

codspeed-hq bot commented Feb 10, 2025

CodSpeed Performance Report

Merging #3773 will not alter performance

Summary

Benchmarks breakdown

blothmann commented Feb 11, 2025

Improve performance of `convert_argument` for list of primitives #3773

Improve performance of `convert_argument` for list of primitives #3773

blothmann commented Feb 10, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Feb 10, 2025 •

edited

Loading

botberry commented Feb 10, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading