Skip to content
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

gh-120144: Make it possible to use sys.monitoring for bdb and make it default for pdb #124533

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

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Sep 25, 2024

This is the most conservative attempt ever to utilize sys.monitoring in bdb and pdb.

Highlights:

  • Full backward compatibility - no changes to test_pdb and test_bdb at all with the new backend
  • bdb will still default to sys.settrace, which keeps all the old behavior. Users can opt-in to the new sys.monitoring backend, and the interface is still the same, even for trace_dispatch (that's how test_bdb passes).
  • New additional and optional interfaces in bdb where user can disable certain events to improve the peformance.
  • pdb.Pdb will use sys.settrace by default too, and is configurable with pdb.Pdb.DEFAULT_BACKEND
  • pdb CLI and breakpoint() uses the monitoring backend and no noticable difference I can observe at this point.

Solution:

Basically, I mimicked the behavior of sys.settrace with sys.monitoring to keep the old behavior as much as possible. But I had the chance to use the new API of sys.monitoring to disable certain events.

Performance:

It's not as impressive as the original proposal, but for the following code:

import time
def f(x):
    # Set breakpoint here
    x *= 2
    return x + 1

def test():
    start_time = time.time()
    for i in range(1000):
        for j in range(1000):
            j + i
    cost = time.time() - start_time
    print(cost)
    f(0)

test()

On my laptop, without debugger, it takes 0.025s. With the new pdb attached (b f then c), it takes the same amount of time, and with Python 3.12 pdb, it takes 1.04s(4100%+ overhead). The performance improvement is significant to say at least.

And as you can tell from the diff, the actual changes to pdb is minimal - just change sys.settrace(tracefunc) to self.start_trace(tracefunc) and sys.settrace(None) to self.stop_trace(). That's what the debugger developers need to do to onboard.

@gaogaotiantian
Copy link
Member Author

And of course we need documentation updates, I will do it later when the feature is accepted and the interface is decided.

@terryjreedy
Copy link
Member

Its after midnight so will test much later today. If all ok using default, will patch IDLE to pass 'monitoring'.

@terryjreedy
Copy link
Member

terryjreedy commented Sep 29, 2024

Ran fine with default backend.

Not fine with backend='monitoring'.
Usually, when I start debugger, stack should one line with bdb.run(). Running a file should top line under that. I think showing bdb.run is an error, but this is what to compare to. With monitoring, there is initially nothing in stack window. Running a file results in 17 lines from threading, idlelib, and bdb. I have to hit 'go' to get to bdb.run + first line. After that, over and step seem to work, but 'go' freezes debugger.

EDIT: The remote execution process crashes because of an unrecoverable exception in the rpc code. Monitoring does not seem to work across the socket connection. Some of the debugger code likely needs a change (as pdb does). (But IDLE does not have to use 'monitoring'.

@gaogaotiantian
Copy link
Member Author

Right - the most important thing is IDLE can simply keep working as it is, but it's also a very important example to test the new mechanism.

I think at least part of the issue is multi-threading. For sys.settrace, it only sets trace on current thread, but sys.monitoring sets events on all threads. That should answer some of the questions. I can patch this PR for thread check and maybe you can take a look after the fix.

@gaogaotiantian
Copy link
Member Author

Hi @terryjreedy , I "fixed" the multi-threading issue. Well by "fixed" I meant making it the same behavior as before. Let me know if you have some time to test it out :)

@gaogaotiantian
Copy link
Member Author

gaogaotiantian commented Oct 17, 2024

With clear_tool_id, I can easily change the events like LINE to local, and that gives me the full speed! The code example runs with basically 0 overhead now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants