Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
markshannon merged 127 commits intopython:mainfromfaster-cpython:pep-669
Apr 12, 2023

Conversation

markshannon
Copy link
Member

@markshannonmarkshannon commentedMar 28, 2023
edited
Loading

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

artemmukhin and erlend-aasland reacted with rocket emoji
@gaogaotiantian
Copy link
Member

Is there another module that behaves like this?

import sysprint(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.monitoringModuleNotFoundError: No module named  'sys.monitoring'; 'sys' is not a package

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

For the record, I had the same issue when I was using it and I was confused.

erlend-aasland reacted with thumbs up emoji

@gvanrossum
Copy link
Member

Is there another module that behaves like this?

import sysprint(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.monitoringModuleNotFoundError: No module named  'sys.monitoring'; 'sys' is not a package

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

But does it matter? There is no promise thatsys.monitoring is a module. It's just a namespace ("let's do more of those!"). You can writeimport sys; sys.monitoring.set_events(...) or you can usefrom sys import modules; modules.set_events(...). I'd say having it be a module is better than having it be a class full of static methods. :-)

@brandtbucher
Copy link
Member

brandtbucher commentedApr 11, 2023
edited
Loading

Is there another module that behaves like this?

import sysprint(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.monitoringModuleNotFoundError: 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:

>>>importast>>> type(ast.sys)<class'module'>>>>importast.sysTraceback (mostrecentcalllast):File"<stdin>",line1,in<module>ModuleNotFoundError:Nomodulenamed'ast.sys';'ast'isnotapackage

🙃

Perhaps a good mental model forsys.monitoring is that the top line of thesys module isimport _something_super_secret as monitoring.

@markshannon
Copy link
MemberAuthor

It is a bit bit weird thatfrom 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
MemberAuthor

@fabioz Thanks for the reproducer.

@markshannon
Copy link
MemberAuthor

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.

@markshannonmarkshannon merged commit411b169 intopython:mainApr 12, 2023
@fabioz
Copy link
Contributor

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

@markshannon
Copy link
MemberAuthor

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 settingf_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
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
Contributor

Choose a reason for hiding this comment

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

Suggested change
Havingstacktop <=0ensuresthatinvalid
valuesarenotvisibletothecycleGC.
Wechoose-1ratherthan0toassistdebugging.*/
Havingstacktop <=0ensuresthatinvalid
valuesarenotvisibletothecycleGC.
Wechoose-1ratherthan0toassistdebugging.*/

DTRACE_FUNCTION_ENTRY();
/* Because this avoids the RESUME,
* we need to update instrumentation */
_Py_Instrument(frame->f_code, tstate->interp);
Copy link
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 agoto exit_unwind on error here.

Copy link
MemberAuthor

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.

erlend-aasland reacted with thumbs up emoji
};

static inline bool
opcode_has_event(int opcode) {
Copy link
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(intopcode) {
opcode_has_event(uint8_topcode)
{

}

static inline bool
is_instrumented(int opcode) {
Copy link
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(intopcode) {
is_instrumented(uint8_topcode)
{

assert(test); \
} while (0)

bool valid_opcode(int opcode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
boolvalid_opcode(intopcode) {
boolvalid_opcode(intopcode)
{

Comment on lines +999 to +1000
_PyInterpreterFrame *frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *target
) {
Copy link
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)
{


#define C_RETURN_EVENTS \
((1 << PY_MONITORING_EVENT_C_RETURN) | \
(1 << PY_MONITORING_EVENT_C_RAISE))
Copy link
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 on lines +1558 to +1559
interp->monitoring_tool_names[tool_id] == NULL
) {
Copy link
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 on lines +869 to +871
else {
return MOST_SIGNIFICANT_BITS[bits];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else {
returnMOST_SIGNIFICANT_BITS[bits];
}
returnMOST_SIGNIFICANT_BITS[bits];


/* Should use instruction metadata for this */
static bool
is_super_instruction(int opcode) {
Copy link
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(intopcode) {
is_super_instruction(uint8_topcode)
{

@fabioz
Copy link
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 settingf_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
Copy link
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).

@markshannonmarkshannon deleted the pep-669 branchApril 12, 2023 12:49
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure⚠️⚠️⚠️

Hi! The buildbotAMD64 Arch Linux TraceRefs 3.x has failed when building commit411b169.

What do you need to do:

  1. Don't panic.
  2. Checkthe buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/484/builds/3091) and take a look at the build logs.
  4. Check if the failure is related to this commit (411b169) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/484/builds/3091

Summary of the results of the build (if available):

Click to see traceback logs
Note:switching to '411b1692811b2ecac59cb0df0f920861c7cf179a'.You are in 'detached HEAD' state. You can look around, make experimentalchanges and commit them, and you can discard any commits you make in thisstate without impacting any branches by switching back to a branch.If you want to create a new branch to retain commits you create, you maydo so (now or later) by using -c with the switch command. Example:  git switch -c <new-branch-name>Or undo this operation with:  git switch -Turn off this advice by setting config variable advice.detachedHead to falseHEAD is now at 411b169281 GH-103082: Implementation of PEP 669: Low Impact Monitoring for CPython (GH-103083)Switched to and reset branch 'main'In file included from Python/instrumentation.c:9:./Include/internal/pycore_object.h:20:35: warning: initialization of ‘PyObject *’ {aka ‘struct _object *’} from ‘int’ makes pointer from integer without a cast [-Wint-conversion]   20 | #define _PyObject_IMMORTAL_REFCNT 999999999|^~~~~~~~~Python/instrumentation.c:19:5: note: in expansion of macro ‘_PyObject_IMMORTAL_REFCNT’   19 |     _PyObject_IMMORTAL_REFCNT,|^~~~~~~~~~~~~~~~~~~~~~~~~./Include/internal/pycore_object.h:20:35: note: (near initialization for ‘DISABLE._ob_next’)   20 | #define _PyObject_IMMORTAL_REFCNT 999999999|^~~~~~~~~Python/instrumentation.c:19:5: note: in expansion of macro ‘_PyObject_IMMORTAL_REFCNT’   19 |     _PyObject_IMMORTAL_REFCNT,|^~~~~~~~~~~~~~~~~~~~~~~~~Python/instrumentation.c:20:5: warning: initialization of ‘PyObject *’ {aka ‘struct _object *’} from incompatible pointer type ‘PyTypeObject *’ {aka ‘struct _typeobject *’} [-Wincompatible-pointer-types]   20 |     &PyBaseObject_Type|^Python/instrumentation.c:20:5: note: (near initialization for ‘DISABLE._ob_prev’)./Include/internal/pycore_object.h:20:35: warning: initialization of ‘PyObject *’ {aka ‘struct _object *’} from ‘int’ makes pointer from integer without a cast [-Wint-conversion]   20 | #define _PyObject_IMMORTAL_REFCNT 999999999|^~~~~~~~~Python/instrumentation.c:25:5: note: in expansion of macro ‘_PyObject_IMMORTAL_REFCNT’   25 |     _PyObject_IMMORTAL_REFCNT,|^~~~~~~~~~~~~~~~~~~~~~~~~./Include/internal/pycore_object.h:20:35: note: (near initialization for ‘_PyInstrumentation_MISSING._ob_next’)   20 | #define _PyObject_IMMORTAL_REFCNT 999999999|^~~~~~~~~Python/instrumentation.c:25:5: note: in expansion of macro ‘_PyObject_IMMORTAL_REFCNT’   25 |     _PyObject_IMMORTAL_REFCNT,|^~~~~~~~~~~~~~~~~~~~~~~~~Python/instrumentation.c:26:5: warning: initialization of ‘PyObject *’ {aka ‘struct _object *’} from incompatible pointer type ‘PyTypeObject *’ {aka ‘struct _typeobject *’} [-Wincompatible-pointer-types]   26 |     &PyBaseObject_Type|^Python/instrumentation.c:26:5: note: (near initialization for ‘_PyInstrumentation_MISSING._ob_prev’)Modules/gcmodule.c:450: visit_decref: Assertion "!_PyObject_IsFreed(op)" failedEnable tracemalloc to get the memory block allocation tracebackobject address  : 0x7f159fa02740object refcount : 1object type     : 0x563902ad8980object type name: dictobject repr     : make: *** [Makefile:1282: Python/frozen_modules/abc.h] Segmentation fault (core dumped)find:‘build’: No such file or directoryfind:‘build’: No such file or directoryfind:‘build’: No such file or directoryfind:‘build’: No such file or directorymake:[Makefile:2676: clean-retain-profile] Error 1 (ignored)

aisk pushed a commit to aisk/cpython that referenced this pull requestApr 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 requestMay 29, 2023
…here it was removed from the struct.See PEP-669 (https://peps.python.org/pep-0669/) and the implementation inpython/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 requestOct 6, 2024
Zheaoli added a commit to Zheaoli/cpython that referenced this pull requestOct 7, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabiozfabiozfabioz left review comments

@iritkatrieliritkatrieliritkatriel left review comments

@artemmukhinartemmukhinartemmukhin left review comments

@erlend-aaslanderlend-aaslanderlend-aasland left review comments

@kumaraditya303kumaraditya303kumaraditya303 left review comments

@gvanrossumgvanrossumgvanrossum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

12 participants
@markshannon@bedevere-bot@drdavella@gvanrossum@nedbat@fabioz@gaogaotiantian@brandtbucher@erlend-aasland@iritkatriel@artemmukhin@kumaraditya303

[8]ページ先頭

©2009-2025 Movatter.jp