This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Add C hook in PyDict_SetItem for debuggers
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, jpe, loewis, piotr.dobrogost, pitrou, rhettinger, sdeibel, vstinner
Priority: normal Keywords: patch

Created on 2009-04-01 16:55 by jpe, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
dicthook.diff jpe, 2009-04-01 16:55 Proof of concept review
testhook.py jpe, 2009-04-01 16:56 Test program
Messages (11)
msg85051 - (view) Author: John Ehresman (jpe) * Date: 2009-04-01 16:55
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.
msg85104 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-04-01 22:41
The patch seems to contain unrelated changes to multiprocessing.
msg85120 - (view) Author: John Ehresman (jpe) * Date: 2009-04-01 23:25
Oops, the multiprocessing changes should not be in the patch
msg85126 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-04-02 01:13
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).
msg85139 - (view) Author: John Ehresman (jpe) * Date: 2009-04-02 02:32
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.
msg85305 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2009-04-03 14:07
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 .
msg148545 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-11-29 07:36
Can this be closed?
msg148551 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-11-29 10:43
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.
msg148595 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2011-11-29 19:11
> 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.
msg148617 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-11-29 21:40
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.
msg365905 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-07 13:28
No activity for 9 years, I close the issue.
History
Date User Action Args
2022-04-11 14:56:47adminsetgithub: 49904
2020-04-07 13:28:52vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg365905

stage: needs patch -> resolved
2020-04-07 12:02:16piotr.dobrogostsetnosy: + piotr.dobrogost
2011-11-29 21:40:35loewissetmessages: + msg148617
2011-11-29 19:11:31vstinnersetnosy: + vstinner
messages: + msg148595
2011-11-29 10:43:11pitrousetnosy: + pitrou
messages: + msg148551
2011-11-29 07:36:26rhettingersetmessages: + msg148545
2011-11-29 06:35:46ezio.melottisetstage: needs patch
type: enhancement
versions: + Python 3.3, - Python 2.7
2009-04-03 14:07:15gvanrossumsetnosy: + gvanrossum
messages: + msg85305
2009-04-02 02:32:42jpesetmessages: + msg85139
2009-04-02 01:13:03rhettingersetnosy: + rhettinger
messages: + msg85126
2009-04-01 23:25:54jpesetnosy: + sdeibel
messages: + msg85120
2009-04-01 22:41:49loewissetnosy: + loewis
messages: + msg85104
2009-04-01 16:56:48jpesetfiles: + testhook.py
2009-04-01 16:55:04jpecreate