Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(46)

#13405: Add DTrace probes

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 6 months ago by jcea
Modified:
4 years, 7 months ago
Reviewers:
martin, jimjjewett
CC:
loewis, rhettinger, jcea, Ronald Oussoren, sasha, AntoinePitrou, wsanchez_wsanchez.net, movement_users.sourceforge.net, Benjamin Peterson, trent, glyph_twistedmatrix.com, lists_durin42.com, laszlo.peter_oracle.com, twl_sauria.com, jbaker_zyasoft.com, joe_alacatialabs.com, danchr_gmail.com, duvall_comfychair.org, swalker, dmalcolm, python_samueljohn.de, mjw_redhat.com, rbu_freitagsrunde.org, garen.parham_gmail.com, xmorel, chris.burroughs_gmail.com, Charles-Fran├žois Natali, lasizoillo_gmail.com, fche_elastic.org, kapil.foss_gmail.com, eric.snow, jmcp_opensolaris.org, scox_redhat.com, Marc.Abramowitz, bkabrda, justin.venus_gmail.com, koobs, francois.dion_gmail.com, retrry_gmail.com, jbeck, nikhil.benesch_gmail.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Patch Set 6 #

Patch Set 7 #

Patch Set 8 #

Patch Set 9 #

Patch Set 10 #

Patch Set 11 #

Patch Set 12 #

Patch Set 13 #

Total comments: 48

Patch Set 14 #

Patch Set 15 #

Patch Set 16 #

Patch Set 17 #

Patch Set 18 #

Patch Set 19 #

Patch Set 20 #

Patch Set 21 #

Patch Set 22 #

Patch Set 23 #

Total comments: 7

Patch Set 24 #

Total comments: 16
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
.hgignore View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +3 lines, -0 lines 1 comment Download
Doc/library/debug.rst View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +2 lines, -1 line 0 comments Download
Doc/library/dtrace.rst View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +183 lines, -0 lines 0 comments Download
Include/code.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +5 lines, -0 lines 1 comment Download
Include/pydtrace.d View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +157 lines, -0 lines 0 comments Download
Include/pydtrace_offsets.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +21 lines, -0 lines 0 comments Download
Include/pydtrace_offsets.sh View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +30 lines, -0 lines 1 comment Download
Lib/test/dtrace_sample.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +73 lines, -0 lines 0 comments Download
Lib/test/test_dtrace.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +344 lines, -0 lines 1 comment Download
Lib/test/test_sys.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +5 lines, -1 line 1 comment Download
Makefile.pre.in View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 6 chunks +49 lines, -0 lines 0 comments Download
Modules/dtracemodule.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +25 lines, -0 lines 0 comments Download
Modules/gcmodule.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +53 lines, -1 line 1 comment Download
Objects/classobject.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 5 chunks +97 lines, -1 line 1 comment Download
Objects/codeobject.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +26 lines, -0 lines 1 comment Download
Objects/typeobject.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 4 chunks +90 lines, -1 line 2 comments Download
Python/ceval.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 9 chunks +173 lines, -0 lines 3 comments Download
Python/sysmodule.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -1 line 1 comment Download
configure View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 35 chunks +213 lines, -31 lines 2 comments Download
configure.in View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +63 lines, -0 lines 0 comments Download
pyconfig.h.in View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +3 lines, -0 lines 0 comments Download
setup.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 8
loewis
Review of the C parts of the patch. http://bugs.python.org/review/13405/diff/3743/12151 File Include/code.h (right): http://bugs.python.org/review/13405/diff/3743/12151#newcode10 Include/code.h:10: #include ...
6 years, 5 months ago #1
loewis
Typos in documentation http://bugs.python.org/review/13405/diff/3743/12150 File Doc/library/dtrace.rst (right): http://bugs.python.org/review/13405/diff/3743/12150#newcode35 Doc/library/dtrace.rst:35: accesible to external scripts. accessible http://bugs.python.org/review/13405/diff/3743/12150#newcode37 ...
6 years, 5 months ago #2
jcea
http://bugs.python.org/review/13405/diff/3743/12150 File Doc/library/dtrace.rst (right): http://bugs.python.org/review/13405/diff/3743/12150#newcode35 Doc/library/dtrace.rst:35: accesible to external scripts. On 2011/12/02 13:45:40, loewis wrote: ...
6 years, 5 months ago #3
jcea
http://bugs.python.org/review/13405/diff/3743/12151 File Include/code.h (right): http://bugs.python.org/review/13405/diff/3743/12151#newcode10 Include/code.h:10: #include "pyconfig.h" On 2011/12/02 00:17:36, loewis wrote: > If ...
6 years, 5 months ago #4
jcea
http://bugs.python.org/review/13405/diff/3743/12165 File Python/ceval.c (right): http://bugs.python.org/review/13405/diff/3743/12165#newcode891 Python/ceval.c:891: { On 2011/12/02 00:17:37, loewis wrote: > Please keep ...
6 years, 5 months ago #5
jcea
6 years, 5 months ago #6
Jim.J.Jewett
API and doc questions http://bugs.python.org/review/13405/diff/6151/Doc/library/dtrace.rst File Doc/library/dtrace.rst (right): http://bugs.python.org/review/13405/diff/6151/Doc/library/dtrace.rst#newcode43 Doc/library/dtrace.rst:43: .. opcode:: function-entry (arg0, arg1, ...
4 years, 7 months ago #7
Jim.J.Jewett
4 years, 7 months ago #8
It seems like these changes are the moral equivalent of a 

    LOG(msg, location)

in a few specific spots, except that the logger is a special protocol that
should normally be lightweight (except for the optimizations that it turns off).
 

I would rather see that as the approximate length of the changes in files that
are not DTRACE-specific.  I think this could be done with conditionally-defined
macros, the way that additions to object headers are.

http://bugs.python.org/review/13405/diff/6152/.hgignore
File .hgignore (right):

http://bugs.python.org/review/13405/diff/6152/.hgignore#newcode67
.hgignore:67: Include/pydtrace.h
Why are these being ignored, if they're part of the patch?

http://bugs.python.org/review/13405/diff/6152/Include/code.h
File Include/code.h (right):

http://bugs.python.org/review/13405/diff/6152/Include/code.h#newcode32
Include/code.h:32: #endif
Is this different from co_lnotab, or just easier to work with?

http://bugs.python.org/review/13405/diff/6152/Include/pydtrace_offsets.sh
File Include/pydtrace_offsets.sh (right):

http://bugs.python.org/review/13405/diff/6152/Include/pydtrace_offsets.sh#new...
Include/pydtrace_offsets.sh:1: #!/bin/sh
Why is there a shell file in Include?

http://bugs.python.org/review/13405/diff/6152/Lib/test/test_dtrace.py
File Lib/test/test_dtrace.py (right):

http://bugs.python.org/review/13405/diff/6152/Lib/test/test_dtrace.py#newcode6
Lib/test/test_dtrace.py:6: if not dtrace.available :
If it were not available, wouldn't the "import dtrace" line have already failed?

http://bugs.python.org/review/13405/diff/6152/Lib/test/test_sys.py
File Lib/test/test_sys.py (right):

http://bugs.python.org/review/13405/diff/6152/Lib/test/test_sys.py#newcode552
Lib/test/test_sys.py:552: import dtrace
Will this module exist even if not compiled in?

http://bugs.python.org/review/13405/diff/6152/Modules/gcmodule.c
File Modules/gcmodule.c (right):

http://bugs.python.org/review/13405/diff/6152/Modules/gcmodule.c#newcode1024
Modules/gcmodule.c:1024: if (PYTHON_GC_DONE_ENABLED())
I would rather that these names talked about DTRACE; I was trying to figure out
why gc_enabled only had to be checked in the new #ifdef branch -- wondering if
it should be in both.

http://bugs.python.org/review/13405/diff/6152/Objects/classobject.c
File Objects/classobject.c (right):

http://bugs.python.org/review/13405/diff/6152/Objects/classobject.c#newcode555
Objects/classobject.c:555: if (PYTHON_INSTANCE_NEW_START_ENABLED()) {
I would prefer that these variables mentioned DTRACE, though the preference
isn't as strong as it was in the garbage collection module.

http://bugs.python.org/review/13405/diff/6152/Objects/codeobject.c
File Objects/codeobject.c (right):

http://bugs.python.org/review/13405/diff/6152/Objects/codeobject.c#newcode107
Objects/codeobject.c:107: #ifdef WITH_DTRACE
If I understand correctly, the line numbers are unpacked and cached at compile
time because the unpacking can't be done within a DTRACE timeslot.  If so, that
really needs to be in a comment here.

Also, doesn't this require changing the magic number, so that WITH_DTRACE and
(without) interpreters won't try to share pyc files?

http://bugs.python.org/review/13405/diff/6152/Objects/typeobject.c
File Objects/typeobject.c (right):

http://bugs.python.org/review/13405/diff/6152/Objects/typeobject.c#newcode756
Objects/typeobject.c:756: PyObject *mod;
A lot of this looks pretty similar across several files... could you write it as
a macro defined in the dtrace header, so that you could just write this as 

DTRACE_ADD_NAMES(PYTHON_INSTANCE_NEW_START);

or some such?  I think it would be much better to have a macro (that might
compile to nothing) at a few spots inside a function, as opposed to
conditionally defining the entire function differently and then delegating.

http://bugs.python.org/review/13405/diff/6152/Objects/typeobject.c#newcode775
Objects/typeobject.c:775: size = _PyObject_VAR_SIZE(type, nitems+1);
Why remove the const declaration?  (Or was that already removed for other
reasons, and this is a merge glitch from using the wrong baseline?)

http://bugs.python.org/review/13405/diff/6152/Python/ceval.c
File Python/ceval.c (right):

http://bugs.python.org/review/13405/diff/6152/Python/ceval.c#newcode687
Python/ceval.c:687: dtrace_entry(PyFrameObject *f)
This pattern is a bit different from the one getting the module name as a
string, but ... the two functions are nearly identical, which suggests that
maybe they could be a written as a macro. (or maybe even both as a single macro,
since they're a bracketing set.... though the brackets are currently not close)

If they are kept separate, does it make sense to combine with some of the
lltrace and/or DEBUG statements, so clean up those #ifdef?

http://bugs.python.org/review/13405/diff/6152/Python/ceval.c#newcode700
Python/ceval.c:700: * Currently a USDT tail-call will not receive the correct
arguments.
What is a "USDT" tail-call?

http://bugs.python.org/review/13405/diff/6152/Python/ceval.c#newcode3847
Python/ceval.c:3847: /* Practically a ripoff of "maybe_call_line_trace"
function. */
Then why is another function needed, as opposed to a conditional emit?  In other
words, why isn't the "get filename/get codename/emit line change" ... stuck
between lines 3913 (resetting the line number) and 3914 (call_trace)

http://bugs.python.org/review/13405/diff/6152/Python/sysmodule.c
File Python/sysmodule.c (left):

http://bugs.python.org/review/13405/diff/6152/Python/sysmodule.c#oldcode1402
Python/sysmodule.c:1402: 
I'm assuming this was not an intentional diff; on my screen, it looks like just
deleting a blank line.

http://bugs.python.org/review/13405/diff/6152/configure
File configure (right):

http://bugs.python.org/review/13405/diff/6152/configure#newcode2610
configure:2610: See \`config.log' for more details" "$LINENO" 5; }
I'm assuming this was a whitespace change.  It would be best to make that set
separately.  I almost missed some real changes at the end, once it looked like
they were "all" this change.

http://bugs.python.org/review/13405/diff/6152/configure#newcode9586
configure:9586: # version HP92453-01 B.11.11.23709.GP, which incorrectly rejects
wait, wasn't this done in 9527 of this same patch chunk?
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7