-
-
Notifications
You must be signed in to change notification settings - Fork 528
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
improve performance of strawberry.lazy()
with relative imports
#2926
Conversation
strawberry/lazy_type.py
Outdated
@@ -73,7 +73,7 @@ def __init__(self, module: str) -> None: | |||
self.package = None | |||
|
|||
if module.startswith("."): | |||
frame = inspect.stack()[2][0] | |||
frame = inspect.currentframe().f_back.f_back |
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.
I didn't bother to check for nulls at every location, since this should only be accessed via .lazy()
and also the previous code would've hit a bounds check in the same condition anyways
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.
nice! we also use sys._getframe(x)
in another place, would you mind trying with that and see if performs the same? 😊
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.
Yupp! It has the same effect - actually _getframe
is in C, so looks like it is even faster. Whipped up a small benchmark to look at the difference:
import time
import inspect
import sys
def run_benchmark(name, fn):
start_time = time.time()
for i in range(1000000):
fn()
end_time = time.time()
duration = end_time - start_time
print(f"{name} completed in {duration}s")
def main():
run_benchmark("inspect.stack()[2]", lambda: inspect.stack()[2])
run_benchmark("inspect.currentframe().f_back.f_back", lambda: inspect.currentframe().f_back.f_back)
run_benchmark("sys._getframe(2)", lambda: sys._getframe(2))
main()
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.
awesome! thanks for that!
Thanks for adding the Here's a preview of the changelog: This release includes a performance improvement to Here's the preview release card for twitter: Here's the tweet text:
|
as a not for myself, I think we should add some benchmark for this :) |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2926 +/- ##
=======================================
Coverage 96.17% 96.17%
=======================================
Files 211 211
Lines 9223 9223
Branches 1704 1704
=======================================
Hits 8870 8870
Misses 227 227
Partials 126 126 |
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.
Thank you so much! 🏃♂️💨
Thanks for contributing to Strawberry! 🎉 You've been invited to join You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67 And don't forget to join our discord server: https://strawberry.rocks/discord 🔥 |
Description
I've been profiling a few tests in my company's codebase and realized that some tests that actually only perform about 1s of business logic take ~12s to run. A large part of that time (~8-9s) is spent resolving forward refs in
strawberry.lazy()
. The largest culprit there was the use ofinspect.stack()
which seems to be incredibly slow.This PR updates the method of retrieving the target stack frame. It seems that the stack frames are stored as a linkedlist anyways, so retrieving the active frame and then working backwards is significantly faster (likely because
.stack()
collects more information for every frame that is unused by strawberry). This one line change took my test down from 12s to 2.5s.Types of Changes
Issues Fixed or Closed by This PR
N/a
Checklist