Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

neoncube2
Copy link
Contributor

@neoncube2 neoncube2 commented Apr 30, 2025

This PR optimizes _connection.py even further. This further fixes #2744.

In the current code, we:

  1. Retrieve the entire stack, by calling getattr(task, "__pw_stack__", inspect.stack(0))
  2. Parse the entire stack
  3. Throw away all of the parsed stack except for the first relevant frame, unless we're tracing (e.g. unless self._tracing_count > 0)

This PR makes it so that we only retrieve and parse as much of the stack as necessary.

Test code:

async def test_speed_test(page: Page) -> None:
    start_time = time.time()

    for i in range(0, 1000):
        await page.evaluate('() => 1 + 1')

    print(time.time() - start_time)

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! :)

@neoncube2 neoncube2 changed the title Even faster stack traces Even faster stack traces for _connection.py Apr 30, 2025
@neoncube2
Copy link
Contributor Author

Ready to rerun tests

@neoncube2
Copy link
Contributor Author

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]:
Copy link
Member

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?

Copy link
Contributor Author

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 calling inspect.stack(0), it would immediately walk the entire stack (essentially, it would call frame.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 (because frame.f_back is only called when we need another frame)

Basically: Calling frame.f_back is more expensive than one might think.

Thanks for the heads up about playwright/_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 by asyncio, but it's actually being set by this package! XD

Let me take a look.

Copy link
Contributor Author

@neoncube2 neoncube2 May 6, 2025

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!) :)

@neoncube2
Copy link
Contributor Author

@mxschmitt I'm still working on this, but I've split off a few simple and easy to review optimizations into #2844

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.

[Feature]: Option to disable inspect.stack() calls for performance optimization
2 participants