- Notifications
You must be signed in to change notification settings - Fork1k
Even faster stack traces for _connection.py#2836
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Even faster stack traces for _connection.py#2836
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ub.com/neoncube2/playwright-python into custom-iterator-for-faster-stack-trace
Ready to rerun tests |
Probably best to wait until#2840 is merged, and then I can run checks locally. |
| def needs_full_stack_trace(self) -> bool: | ||
| return self._tracing_count > 0 | ||
| def get_frame_info(self) -> Iterator[FrameInfo]: |
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.
Just thinking out loud - why does it need to be an Iterator in order to get the faster stacktraces? I assume that we call it only when we actually need it in line 546 and 566?
Also should we maybe use this similar function in playwright/_impl/_sync_base.pyplaywright/_impl/_sync_base.py:108` ? Then sync users - what the majority is - would benefit as well?
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.
As to your question: In the original code, when we were callinginspect.stack(0), it would immediately walk the entire stack (essentially, it would callframe.f_back in a loop until there were no more frames). This is pretty expensive. By using an iterator, we delay walking the stack until we need it, and then we can only walk part of the stack (becauseframe.f_back is only called when we need another frame)
Basically: Callingframe.f_back is more expensive than one might think.
Thanks for the heads up aboutplaywright/_impl/_sync_base.pyplaywright/_impl/_sync_base.py:108! :) No wonder I couldn't find any information about__pw_stack__... I thought it was being set byasyncio, but it's actually being set by this package! XD
Let me take a look.
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.
@mxschmitt Now that I'm working on this again, I see what you mean about the iterator. I think that once I merge the logic in_sync_base.py and_connection.py, this PR should be a lot cleaner (and faster!) :)
@mxschmitt I'm still working on this, but I've split off a few simple and easy to review optimizations into#2844 |
neoncube2 commentedMay 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@mxschmitt While continuing to work on this, I've discovered something interesting.
Interestingly, for all of the tests in the Any thoughts? |
I wonder when it was introduced if it was any different back then:c05cad2#diff-291261f4050ff453e2c661edf96c77af65a779b75815708d924809ee49d5dc64 - would be great to find out if it was used back in the days and not anymore or if our tests don't have the coverage. |
neoncube2 commentedJun 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@mxschmitt Similarly, it might be nice to know if sending
|
I think that probably the best way to move this PR forward would be to modify the monkeypatch by@SichangHe (#2744 (comment)), as it looks like it preserved the best of all worlds and has the least chance of breakage :) I'm thinking it would probably make sense to open a new PR for that, but let's keep this PR open for now. |
Uh oh!
There was an error while loading.Please reload this page.
This PR optimizes
_connection.pyeven further. This furtherfixes#2744.In the current code, we:
getattr(task, "__pw_stack__", inspect.stack(0))self._tracing_count > 0)This PR makes it so that we only retrieve and parse as much of the stack as necessary.
Test code:
Before this PR: 4s
With this PR: 1.75s
Speed improvement: 2.3x
I'd like add tests for when
self._is_internal_type = True, but I'm not sure the best way to go about constructing an internal type and then triggering an exception in it. (Any suggestions?)Similarly, I'd like to add tests for when
getattr(task, "__pw_stack__")returns a value, but again, I'm not sure when this occurs. If anyone has suggestions, please let me know! :)