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

Add C hook in PyDict_SetItem for debuggers #49904

Closed
jpe mannequin opened this issue Apr 1, 2009 · 13 comments
Closed

Add C hook in PyDict_SetItem for debuggers #49904

jpe mannequin opened this issue Apr 1, 2009 · 13 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@jpe
Copy link
Mannequin

jpe mannequin commented Apr 1, 2009

BPO 5654
Nosy @gvanrossum, @loewis, @rhettinger, @pitrou, @vstinner
Files
  • dicthook.diff: Proof of concept
  • testhook.py: Test program
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2020-04-07.13:28:52.509>
    created_at = <Date 2009-04-01.16:55:03.842>
    labels = ['interpreter-core', 'type-feature']
    title = 'Add C hook in PyDict_SetItem for debuggers'
    updated_at = <Date 2020-04-07.13:28:52.508>
    user = 'https://bugs.python.org/jpe'

    bugs.python.org fields:

    activity = <Date 2020-04-07.13:28:52.508>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-04-07.13:28:52.509>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2009-04-01.16:55:03.842>
    creator = 'jpe'
    dependencies = []
    files = ['13549', '13550']
    hgrepos = []
    issue_num = 5654
    keywords = ['patch']
    message_count = 11.0
    messages = ['85051', '85104', '85120', '85126', '85139', '85305', '148545', '148551', '148595', '148617', '365905']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'loewis', 'rhettinger', 'jpe', 'sdeibel', 'pitrou', 'vstinner', 'piotr.dobrogost']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue5654'
    versions = ['Python 3.3']

    @jpe
    Copy link
    Mannequin Author

    jpe mannequin commented Apr 1, 2009

    I'm interested in adding support for debugger watchpoint's in the core.
    A watchpoint would cause the debugger to stop when a value in a
    namespace changes. This hook would be called when PyDict_SetItem &
    PyDict_DelItem is invoked. I realize that this does not cover function
    local variables, but I'm more interested in instance attributes and globals.

    This is a proof of concept patch; I wanted to see if it would work and
    get some initial feedback. I think the performance implications are
    minimal in the case where nothing is watched, though the performance of
    sample callback in _debuggerhooks can be improved. An issue may be if
    this is used outside a debugger implementation to implement an observer
    pattern -- I'd like to discourage it, but worry that it will be used
    since it's there, rather like how the settrace tracer gets used.

    If there's interest in this, I will clean this up and add watchpoints to
    pdb.

    @jpe jpe mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Apr 1, 2009
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Apr 1, 2009

    The patch seems to contain unrelated changes to multiprocessing.

    @jpe
    Copy link
    Mannequin Author

    jpe mannequin commented Apr 1, 2009

    Oops, the multiprocessing changes should not be in the patch

    @rhettinger
    Copy link
    Contributor

    This is a critical path in Python and it should be kept as clean as
    possible. If something like this goes in, it should be #ifdef'd out by
    default; otherwise, we have everyone paying the price for a feature that
    almost no one will use.

    Strong -1 against this being in the default build.

    Also, I'm not convinced that the idea itself is even that useful. Why
    dictionary sets/dels and not list assignments and whatnot. By itself,
    this one watchpoint hook provides very little utility. We've already
    cluttered the codebase with various trace and timing options. If
    something else were to be included, it should be part of a larger, well
    thought out approach (like what is being done for DTrace).

    @jpe
    Copy link
    Mannequin Author

    jpe mannequin commented Apr 2, 2009

    My hope is that the runtime performance cost will be negligible but if
    it isn't, it probably shouldn't go in. The issue with not putting it in
    another build is that many python debugger users won't want to
    recompile, so I see it as being of limited use if it's not in the
    default build.

    My experience with watchpoints in C debuggers (gdb) is they can be the
    difference between finding a bug and not -- I recall finding ref
    counting bugs only because I could watch the ref count and break on the
    code that modifies it.

    I would be willing to try and generalize this and support more hooks if
    there is some interest in it, though watching namespaces is probably the
    greatest payoff. I think that if will be difficult to remove the hooks
    currently in the default build because of backward compatibility issues.

    @gvanrossum
    Copy link
    Member

    John, I'm adding a +0 to cancel out Raymond's -1. I've read your
    motivation and, like you, hope/expect that benchmarking will show this
    has no real impact. But I need data to decide. Once there are
    benchmark results I'll revise my view. A good place to start looking
    for benchmarks might be the Performance section of
    http://code.google.com/p/unladen-swallow/wiki/ProjectPlan .

    @ezio-melotti ezio-melotti added the type-feature A feature request or enhancement label Nov 29, 2011
    @rhettinger
    Copy link
    Contributor

    Can this be closed?

    @pitrou
    Copy link
    Member

    pitrou commented Nov 29, 2011

    I see no reason to close until benchmark results prove it decreases real-life performance. Looking at the patch, the cost in the normal case is a single pointer comparison with NULL, something very cheap.

    @vstinner
    Copy link
    Member

    the cost in the normal case is a single pointer comparison with NULL,
    something very cheap.

    The Linux kernel patchs dynamically the code: disabling a probe writes NOP instruction in memory to avoid the cost of the test. Probes are completly disabled by default.

    See kernel markers (introduced in Linux 2.6.24) and the more recent kernel tracepoints (introduced in Linux 2.6.28).
    http://lwn.net/Articles/300992/

    If something like this goes in, it should be #ifdef'd out
    by default

    I agree.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Nov 29, 2011

    I think it's besides the point of this patch to ifdef it out. If John wants a version of Python with this enabled, he can well enough build one without our permission. So it would be useful only if it was always available, and perhaps properly integrated into pdb.

    IOW, I'm +1 for the patch. Having actual benchmarks would demonstrating the (expected) outcome (i.e. no measurable effect) would still be required to advance this.

    @vstinner
    Copy link
    Member

    vstinner commented Apr 7, 2020

    No activity for 9 years, I close the issue.

    @vstinner vstinner closed this as completed Apr 7, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @chadfurman
    Copy link

    I wish this wasn't a closed issue. It's not like a PR was abandoned. The issue is still real and very much affects the professional adoption of python across enterprise environments globally. Watchpoints are a necessary feature in modern IDEs for debugging, and if I understand correctly this sort of hook is necessary to enable watchpoints in a performant way

    @gvanrossum
    Copy link
    Member

    @chadfurman Not to worry, this is being done for 3.12. See #91052

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants