playwright-python icon indicating copy to clipboard operation
playwright-python copied to clipboard

Even faster stack traces for _connection.py

Open neoncube2 opened this issue 7 months ago • 7 comments

This PR optimizes _connection.py even further. This further fixes https://github.com/microsoft/playwright-python/issues/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 avatar Apr 30 '25 06:04 neoncube2

Ready to rerun tests

neoncube2 avatar May 02 '25 07:05 neoncube2

Probably best to wait until https://github.com/microsoft/playwright-python/pull/2840 is merged, and then I can run checks locally.

neoncube2 avatar May 03 '25 10:05 neoncube2

@mxschmitt I'm still working on this, but I've split off a few simple and easy to review optimizations into https://github.com/microsoft/playwright-python/pull/2844

neoncube2 avatar May 08 '25 08:05 neoncube2

@mxschmitt While continuing to work on this, I've discovered something interesting.

  1. In _sync_base.py, we set __pw_stack__ and __pw_stack_trace__
  2. In _connection.py, we check to see if the current task has __pw_stack__ or __pw_stack_trace__ set, and if not, we generate a new stack trace to use.

Interestingly, for all of the tests in the tests\ folder, __pw_stack__ and __pw_stack_trace__ are always None whenever they're retrieved in _connection.py. This makes me wonder if we should stop setting __pw_stack__ and __pw_stack_trace__ in _sync_base.py (because these values are never being used) or if we just need to better tests to cover this case.

Any thoughts?

neoncube2 avatar May 14 '25 16:05 neoncube2

I wonder when it was introduced if it was any different back then: https://github.com/microsoft/playwright-python/commit/c05cad257a026d450644aa3baae6f9d81bd1b3ad#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.

mxschmitt avatar Jun 04 '25 07:06 mxschmitt

@mxschmitt Similarly, it might be nice to know if sending apiName and internal as part of the metadata is necessary or was just added because it was nice to have: https://github.com/microsoft/playwright-python/blob/e87e340fae92cb52f98dacdd80bb55c04e9bc4e3/playwright/_impl/_connection.py#L351

neoncube2 avatar Jun 05 '25 04:06 neoncube2

I think that probably the best way to move this PR forward would be to modify the monkeypatch by @SichangHe (https://github.com/microsoft/playwright-python/issues/2744#issuecomment-2939291173), 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.

neoncube2 avatar Jun 05 '25 05:06 neoncube2