Add C hook in PyDict_SetItem for debuggers

[jpe]

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]

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.

[loewis]

The patch seems to contain unrelated changes to multiprocessing.

[jpe]

Oops, the multiprocessing changes should not be in the patch

[rhettinger]

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]

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]

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 .

[rhettinger]

Can this be closed?

[pitrou]

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]

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]

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]

No activity for 9 years, I close the issue.

[chadfurman]

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]

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