-
-
Notifications
You must be signed in to change notification settings - Fork 608
Add custom is_awaitable to test perf #4035
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
Conversation
Reviewer's GuideAdds a new optimized is_awaitable helper and wires it into the async and sync execution paths to reduce overhead when checking awaitables during GraphQL execution, exposing it via the execution utilities package. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4035 +/- ##
=======================================
Coverage 94.30% 94.30%
=======================================
Files 541 543 +2
Lines 35641 35653 +12
Branches 1884 1885 +1
=======================================
+ Hits 33610 33622 +12
Misses 1714 1714
Partials 317 317 🚀 New features to boost your workflow:
|
Merging this PR will improve performance by 10.52%
Performance Changes
Comparing |
2241527 to
8cbd5c3
Compare
Instead of duplicating the awaitable detection logic from graphql-core, import and use their is_awaitable function as a fallback. This keeps the fast path for common non-awaitable types while reducing code duplication and ensuring we stay in sync with graphql-core.
|
Thanks for adding the Here's a preview of the changelog: Improved execution performance by up to 10% by adding an optimized This optimization reduces overhead when processing large result sets containing mostly basic values by avoiding expensive awaitable checks for types that are known to never be awaitable. Here's the tweet text: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `strawberry/execution/is_awaitable.py:18-27` </location>
<code_context>
+
+# Common synchronous types that are never awaitable
+# Using a frozenset for O(1) lookup
+_NON_AWAITABLE_TYPES: frozenset[type] = frozenset(
+ {
+ type(None),
+ bool,
+ int,
+ float,
+ str,
+ bytes,
+ bytearray,
+ list,
+ tuple,
+ dict,
+ set,
+ frozenset,
+ }
+)
+
</code_context>
<issue_to_address>
**suggestion (performance):** Expand the fast-path set to cover additional common primitive/iterable types
You’ve covered many common built-ins, but types like `complex`, `range`, and `memoryview` are also frequent non-awaitable primitives/iterables that will still hit `graphql_core_is_awaitable`. Consider adding them to `_NON_AWAITABLE_TYPES` to further reduce fall-through in numeric/iterator-heavy code paths without changing behavior, since these types can’t be awaitable.
Suggested implementation:
```python
# Common synchronous types that are never awaitable
# Using a frozenset for O(1) lookup
_NON_AWAITABLE_TYPES: frozenset[type] = frozenset(
{
type(None),
bool,
int,
float,
complex,
str,
bytes,
bytearray,
memoryview,
range,
list,
tuple,
dict,
set,
frozenset,
}
)
```
If `_NON_AWAITABLE_TYPES` is referenced elsewhere in this file (e.g., inside `optimized_is_awaitable`), no further changes are required. If a different constant name was used in other parts of the PR, those references should be updated to `_NON_AWAITABLE_TYPES` for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryAdded an optimized
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Schema
participant execute/execute_sync
participant optimized_is_awaitable
participant graphql_core_is_awaitable
Client->>Schema: execute() or execute_sync()
Schema->>Schema: Import optimized_is_awaitable
Schema->>execute/execute_sync: Call with is_awaitable=optimized_is_awaitable
loop For each result value in GraphQL execution
execute/execute_sync->>optimized_is_awaitable: Check if value is awaitable
alt Value is primitive (int/str/list/dict/etc)
optimized_is_awaitable->>optimized_is_awaitable: type(value) in _NON_AWAITABLE_TYPES
optimized_is_awaitable-->>execute/execute_sync: Return False (fast path)
else Value is other type
optimized_is_awaitable->>graphql_core_is_awaitable: Delegate to graphql-core
graphql_core_is_awaitable-->>optimized_is_awaitable: Return awaitable check result
optimized_is_awaitable-->>execute/execute_sync: Return result
end
end
execute/execute_sync-->>Schema: Return execution result
Schema-->>Client: Return GraphQL response
|
bellini666
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.


Summary by Sourcery
Introduce an optimized awaitable detection helper and wire it into schema execution to improve performance on large, mostly-synchronous result sets.
Enhancements:
Chores: