Skip to content

Skip generic check in StrawberryAnnotation.resolve() without a type definition - #4790

Merged
bellini666 merged 1 commit into
strawberry-graphql:mainfrom
fernandoflorez:perf/annotation-resolve-short-circuit
Nov 5, 2026
Merged

bellini666 merged 1 commit into
strawberry-graphql:mainfrom
fernandoflorez:perf/annotation-resolve-short-circuit

Conversation

@fernandoflorez

@fernandoflorez fernandoflorez commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

StrawberryAnnotation.resolve() checks _is_type_generic(resolved) before
type_definition is not None. When there's no type definition the result is
always false, but the generic check still runs.

That happens a lot: convert_arguments reads argument.type (which calls
resolve() with no type definition) for every argument on every field
execution. Swapping the two conditions skips the check in that case; the
behavior doesn't change, since without a type definition resolve() just
returns the cached type.

In a list-heavy query I was profiling (fields with arguments on each item),
this gave a 2–8% speedup in execution time.

Added a test checking that _is_type_generic isn't called when there's no
type definition.

Summary by Sourcery

Optimize annotation resolution for argument conversion without changing its behavior.

Enhancements:

  • Improve annotation resolution performance by skipping generic type checks when no type definition is available.

Tests:

  • Add coverage confirming generic checks are skipped when resolving annotations without a type definition.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Thanks for adding the RELEASE.md file!

Below is the changelog that will be used for the release.


This release improves the performance of argument conversion by skipping an
unnecessary generic type check when an annotation is resolved without a type
definition.

This release was contributed by @fernandoflorez in #4790

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Sourcery assessment

Approved.


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

@greptile-apps

greptile-apps Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] This PR appears safe to merge.

Summary

This PR skips _is_type_generic when StrawberryAnnotation.resolve() receives no type definition.

  • Annotation resolution skips generic checks without a type definition.

No actionable issues were found.

Reviews (1) · Last reviewed commit: "Skip generic check in StrawberryAnnotati..." · Reviewed by Greptile

@bellini666 bellini666 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small and effective, love it ❤️

@bellini666
bellini666 merged commit 32133be into strawberry-graphql:main Nov 5, 2026
97 of 100 checks passed
@codspeed

codspeed Bot commented Nov 5, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 4 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 71 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing fernandoflorez:perf/annotation-resolve-short-circuit (96efac1) with main (02770ca)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants