GH-103082: Implementation of PEP 669: Low Impact Monitoring for CPython by markshannon · Pull Request #103083 · python/cpython · GitHub
Skip to content

GH-103082: Implementation of PEP 669: Low Impact Monitoring for CPython - #103083

Merged
markshannon merged 127 commits into
python:mainfrom
faster-cpython:pep-669
Apr 12, 2023
Merged

GH-103082: Implementation of PEP 669: Low Impact Monitoring for CPython#103083
markshannon merged 127 commits into
python:mainfrom
faster-cpython:pep-669

Conversation

@markshannon

@markshannon markshannon commented Mar 28, 2023

Copy link
Copy Markdown
Member

This implements PEP 669.
There are a couple of things missing, but no harm in early review.

@gvanrossum

Copy link
Copy Markdown
Member

@brandtbucher

brandtbucher commented Apr 11, 2023

Copy link
Copy Markdown
Member

Is there another module that behaves like this?

import sys

print(type(sys.monitoring))

import sys.monitoring

produces:

<class 'module'>
Traceback (most recent call last):
  File "/Users/nedbatchelder/coverage/trunk/../lab/pep669.py", line 5, in <module>
    import sys.monitoring
ModuleNotFoundError: No module named  'sys.monitoring'; 'sys' is not a package

It's a module but I can't import it directly.

This happens basically whenever a module imports another module:

>>> import ast
>>> type(ast.sys)
<class 'module'>
>>> import ast.sys
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named  'ast.sys'; 'ast' is not a package

🙃

Perhaps a good mental model for sys.monitoring is that the top line of the sys module is import _something_super_secret as monitoring.

@markshannon

Copy link
Copy Markdown
Member Author

It is a bit bit weird that from sys import monitoring as m doesn't work.
We should probably fix that.

I expect to (hopefully before 3.12) use some sort of lazy loading to avoid creating the monitoring object if it isn't needed.

@markshannon

Copy link
Copy Markdown
Member Author

@fabioz Thanks for the reproducer.

@markshannon

Copy link
Copy Markdown
Member Author

OK. I'm merging this.

I think this is stable enough to merge, and we can probably get better bug reports with this merged than on a branch.

@markshannon
markshannon merged commit 411b169 into python:main Apr 12, 2023
@fabioz

fabioz commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

@markshannon from the fix you seem to have put there, it'd still fail if the user set the tracing to None and then back to the actual trace function...

@markshannon

Copy link
Copy Markdown
Member Author

I think that is the current behavior, is it not?

If you restart tracing, then you want a line event for the current line.
At least, pdb seems to want that. If I don't clear the "last traced line", when setting f_trace, then the pdb tests fail.

TBH, it is all an undocumented black box, so some guess work is required.
It seems the best we can do is add test cases, so if you have any more tests I'd be grateful.

@erlend-aasland

Copy link
Copy Markdown
Contributor

Darn, I was just about to complete my second review.

$ ./python.exe -m test -R : test_monitoring  # <= fails

Comment on lines +142 to +144
Having stacktop <= 0 ensures that invalid
values are not visible to the cycle GC.
We choose -1 rather than 0 to assist debugging. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Having stacktop <= 0 ensures that invalid
values are not visible to the cycle GC.
We choose -1 rather than 0 to assist debugging. */
Having stacktop <= 0 ensures that invalid
values are not visible to the cycle GC.
We choose -1 rather than 0 to assist debugging. */

Comment thread Python/ceval.c
DTRACE_FUNCTION_ENTRY();
/* Because this avoids the RESUME,
* we need to update instrumentation */
_Py_Instrument(frame->f_code, tstate->interp);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value of _Py_Instrument is ignored here. Since it might set an exception, I'd expect a goto exit_unwind on error here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are already handling an exception here, so the exception isn't lost, it replaces the thrown exception.
The question is whether to raise it back to the caller, or to inject into the coroutine/generator?

If unwinding were done in a separate function from evaluation, then the thrown exception would unwind the stack, then the new exception would be raised on continued execution, which would be better.

I'm inclined to leave it for now, and let it get fixed as a side effect of separating evaluation and unwinding.

Comment thread Python/instrumentation.c
};

static inline bool
opcode_has_event(int opcode) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
opcode_has_event(int opcode) {
opcode_has_event(uint8_t opcode)
{

Comment thread Python/instrumentation.c
}

static inline bool
is_instrumented(int opcode) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
is_instrumented(int opcode) {
is_instrumented(uint8_t opcode)
{

Comment thread Python/instrumentation.c
assert(test); \
} while (0)

bool valid_opcode(int opcode) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool valid_opcode(int opcode) {
bool valid_opcode(int opcode)
{

Comment thread Python/instrumentation.c
Comment on lines +999 to +1000
_PyInterpreterFrame *frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *target
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_PyInterpreterFrame *frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *target
) {
_PyInterpreterFrame *frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *target)
{

Comment thread Python/instrumentation.c

#define C_RETURN_EVENTS \
((1 << PY_MONITORING_EVENT_C_RETURN) | \
(1 << PY_MONITORING_EVENT_C_RAISE))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(1 << PY_MONITORING_EVENT_C_RAISE))
(1 << PY_MONITORING_EVENT_C_RAISE))

Comment thread Python/instrumentation.c
Comment on lines +1558 to +1559
interp->monitoring_tool_names[tool_id] == NULL
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
interp->monitoring_tool_names[tool_id] == NULL
) {
interp->monitoring_tool_names[tool_id] == NULL)
{

Comment thread Python/instrumentation.c
Comment on lines +869 to +871
else {
return MOST_SIGNIFICANT_BITS[bits];
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
else {
return MOST_SIGNIFICANT_BITS[bits];
}
return MOST_SIGNIFICANT_BITS[bits];

Comment thread Python/instrumentation.c

/* Should use instruction metadata for this */
static bool
is_super_instruction(int opcode) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
is_super_instruction(int opcode) {
is_super_instruction(uint8_t opcode)
{

@fabioz

fabioz commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

I think that is the current behavior, is it not?

If you restart tracing, then you want a line event for the current line. At least, pdb seems to want that. If I don't clear the "last traced line", when setting f_trace, then the pdb tests fail.

TBH, it is all an undocumented black box, so some guess work is required. It seems the best we can do is add test cases, so if you have any more tests I'd be grateful.

Ok, I'll try another round of the debugger tests to see if there's more breakage -- that previous issue didn't really let me get further, so, I'll check and report back -- with bugs in the tracker if that's the case I guess ;)

@fabioz

fabioz commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

@markshannon I just checked and changing the tracing shouldn't duplicate line events in the new tracer (it should only report when the line actually changes).

I created #103471 with a test case for this (which works in Python 3.11 and fails in the current master).

@markshannon
markshannon deleted the pep-669 branch April 12, 2023 12:49
@bedevere-bot

Copy link
Copy Markdown

aisk pushed a commit to aisk/cpython that referenced this pull request Apr 18, 2023
… CPython (pythonGH-103083)

* The majority of the monitoring code is in instrumentation.c

* The new instrumentation bytecodes are in bytecodes.c

* legacy_tracing.c adapts the new API to the old sys.setrace and sys.setprofile APIs
scoder added a commit to cython/cython that referenced this pull request May 29, 2023
…here it was removed from the struct.

See PEP-669 (https://peps.python.org/pep-0669/) and the implementation in python/cpython#103083.
There is more to be done to properly support PEP-669, but this makes it compile.

See #5450
Zheaoli added a commit to Zheaoli/cpython that referenced this pull request Oct 6, 2024
Zheaoli added a commit to Zheaoli/cpython that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.