-
-
Notifications
You must be signed in to change notification settings - Fork 603
⚡️ Speed up function print_field_directives by 269% in PR #4019 (fix-ruff-py310)
#4026
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
⚡️ Speed up function print_field_directives by 269% in PR #4019 (fix-ruff-py310)
#4026
Conversation
The optimized code achieves a **268% speedup** by eliminating expensive nested operations and reducing attribute lookups in hot loops. **Key optimizations:** 1. **Eliminated Generator Overhead**: The original code used nested generators with `any()` checks inside the `"".join()` call, creating ~3.1ms of overhead (98% of runtime). The optimized version pre-filters matching directives into a list, avoiding the expensive generator chain. 2. **Reduced Attribute Lookups**: Localized frequently accessed attributes (`name_converter`, `getattr`, `UNSET`, `fields`) to avoid repeated attribute resolution in tight loops. This is particularly effective for the dictionary comprehension in `print_schema_directive`. 3. **Optimized Location Checking**: Replaced the expensive `any()` + list comprehension pattern with direct `is` comparisons against pre-stored location constants (`FIELD_DEF`, `INPUT_FIELD_DEF`), eliminating list creation overhead. 4. **Changed Control Flow**: Used `elif` chains instead of separate `if` statements in type checking, allowing early exits and reducing redundant isinstance calls. 5. **Minimized Function Call Overhead**: Cached the `append` method reference to avoid repeated method lookups in the directive filtering loop. **Performance characteristics by test case:** - **Small schemas** (1-2 directives): 15-32% faster due to reduced overhead - **Large schemas** (999 directives): 427% faster - the list-based approach scales much better than the generator chain - **Mixed directive types**: 20-42% faster due to more efficient filtering logic The optimization is most effective for schemas with many directives or complex field definitions, where the original's quadratic-like behavior from nested generators becomes a bottleneck.
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.
Greptile Overview
Summary
This PR applies performance optimizations to print_field_directives and print_schema_directive functions, achieving a reported 269% speedup through reduced attribute lookups and elimination of generator overhead.
Key changes:
- Pre-filters directives into a list instead of using nested generators with
any()checks - Caches frequently accessed attributes (
name_converter,getattr,UNSET,fields) to avoid repeated lookups - Changes if-elif chain for type checking to enable early exits
- Uses identity comparison (
is) instead of equality (==) for Location enum checks
Issue found:
- The use of
isfor enum comparison on line 216 is risky - while it works for in-process comparisons, it relies on object identity rather than equality, which could break in edge cases like module reloads or when enums are instantiated differently
Confidence Score: 3/5
- This PR has solid performance improvements but contains a logical bug with enum identity checks that could cause silent failures in edge cases
- The optimizations are well-intentioned and backed by extensive regression testing (21 tests passing), but the use of
isinstead of==for enum comparison is a correctness issue. While enums in Python typically work with identity checks due to caching, this is not guaranteed by the language spec and can fail in edge cases. The rest of the optimizations (caching attributes, using lists instead of generators, elif chains) are safe and beneficial. - strawberry/printer/printer.py requires attention - the enum identity comparison on line 216 should use equality (
==) instead of identity (is)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| strawberry/printer/printer.py | 3/5 | Performance optimizations applied to reduce attribute lookups and generator overhead; contains a potential logical bug in directive filtering |
Sequence Diagram
sequenceDiagram
participant Caller
participant print_field_directives
participant print_schema_directive
participant print_schema_directive_params
participant PrintExtras
Caller->>print_field_directives: field, schema, extras
alt field is None
print_field_directives-->>Caller: ""
end
print_field_directives->>print_field_directives: Cache Location.FIELD_DEFINITION & INPUT_FIELD_DEFINITION
print_field_directives->>print_field_directives: Pre-filter directives into list
loop for each directive
print_field_directives->>print_field_directives: Check locations with identity comparison (is)
alt location matches FIELD_DEF or INPUT_FIELD_DEF
print_field_directives->>print_field_directives: Append to matching_directives
end
end
loop for each matching directive
print_field_directives->>print_schema_directive: directive, schema, extras
print_schema_directive->>print_schema_directive: Cache name_converter, getattr, UNSET, fields
print_schema_directive->>print_schema_directive: Build params dict with cached functions
print_schema_directive->>print_schema_directive: Unwrap StrawberryContainer types
print_schema_directive->>print_schema_directive: Check types with elif chain
print_schema_directive->>PrintExtras: Add directive and types to extras
print_schema_directive-->>print_field_directives: formatted directive string
end
print_field_directives-->>Caller: joined directive strings
1 file reviewed, 1 comment
| for directive in field_directives: | ||
| locations = directive.__strawberry_directive__.locations # type: ignore | ||
| for location in locations: | ||
| if location is FIELD_DEF or location is INPUT_FIELD_DEF: |
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.
logic: using is for enum comparison is dangerous - Location.FIELD_DEFINITION is an Enum, and while the optimization works in the same process, is checks object identity not equality. If the Location enum is instantiated differently (e.g., across module reloads or in certain edge cases), this will fail silently.
| if location is FIELD_DEF or location is INPUT_FIELD_DEF: | |
| if location == FIELD_DEF or location == INPUT_FIELD_DEF: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: strawberry/printer/printer.py
Line: 216:216
Comment:
**logic:** using `is` for enum comparison is dangerous - `Location.FIELD_DEFINITION` is an Enum, and while the optimization works in the same process, `is` checks object identity not equality. If the Location enum is instantiated differently (e.g., across module reloads or in certain edge cases), this will fail silently.
```suggestion
if location == FIELD_DEF or location == INPUT_FIELD_DEF:
```
How can I resolve this? If you propose a fix, please make it concise.
⚡️ This pull request contains optimizations for PR #4019
If you approve this dependent PR, these changes will be merged into the original PR branch
fix-ruff-py310.📄 269% (2.69x) speedup for
print_field_directivesinstrawberry/printer/printer.py⏱️ Runtime :
625 microseconds→169 microseconds(best of43runs)📝 Explanation and details
The optimized code achieves a 268% speedup by eliminating expensive nested operations and reducing attribute lookups in hot loops.
Key optimizations:
Eliminated Generator Overhead: The original code used nested generators with
any()checks inside the"".join()call, creating ~3.1ms of overhead (98% of runtime). The optimized version pre-filters matching directives into a list, avoiding the expensive generator chain.Reduced Attribute Lookups: Localized frequently accessed attributes (
name_converter,getattr,UNSET,fields) to avoid repeated attribute resolution in tight loops. This is particularly effective for the dictionary comprehension inprint_schema_directive.Optimized Location Checking: Replaced the expensive
any()+ list comprehension pattern with directiscomparisons against pre-stored location constants (FIELD_DEF,INPUT_FIELD_DEF), eliminating list creation overhead.Changed Control Flow: Used
elifchains instead of separateifstatements in type checking, allowing early exits and reducing redundant isinstance calls.Minimized Function Call Overhead: Cached the
appendmethod reference to avoid repeated method lookups in the directive filtering loop.Performance characteristics by test case:
The optimization is most effective for schemas with many directives or complex field definitions, where the original's quadratic-like behavior from nested generators becomes a bottleneck.
✅ Correctness verification report:
🌀 Generated Regression Tests and Runtime
To edit these changes
git checkout codeflash/optimize-pr4019-2025-10-12T15.38.19and push.