Skip to content

gh-107674: Improve performance of sys.settrace #114986

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
markshannon merged 10 commits into from
Feb 28, 2024
Merged

gh-107674: Improve performance of sys.settrace #114986

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6e53f53
Improve performance of sys.settrace
gaogaotiantian Feb 4, 2024
a42d7c8
📜🤖 Added by blurb_it.
blurb-it[bot] Feb 4, 2024
d5b1dec
Do not instrument code when tracing
gaogaotiantian Feb 5, 2024
2a1eefb
Reduce extra branch and save some operations
gaogaotiantian Feb 5, 2024
cb09c55
Change the comment to assertion
gaogaotiantian Feb 5, 2024
410a07a
Merge branch 'main' into sys-settrace-optimize
gaogaotiantian Feb 24, 2024
74d3375
Add missing instr change
gaogaotiantian Feb 26, 2024
47ff554
Do not deopt RESUME_CHECK if it's tracing
gaogaotiantian Feb 26, 2024
79f43b9
Regen executor_cases
gaogaotiantian Feb 26, 2024
0c72ada
Clean up bytecode code
gaogaotiantian Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Do not instrument code when tracing
  • Loading branch information
@gaogaotiantian
gaogaotiantian committed Feb 5, 2024
commit d5b1dec2a57999853cd20efe8bb3ef65e479517b
17 changes: 10 additions & 7 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,16 +148,19 @@ dummy_func(
~_PY_EVAL_EVENTS_MASK;
uintptr_t code_version = _PyFrame_GetCode(frame)->_co_instrumentation_version;
assert((code_version & 255) == 0);
if (code_version != global_version) {
int err = _Py_Instrument(_PyFrame_GetCode(frame), tstate->interp);
ERROR_IF(err, error);
next_instr = this_instr;
if (tstate->tracing == 0) {
Copy link
Copy Markdown
Member

@markshannon markshannon Feb 26, 2024

Choose a reason for hiding this comment

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

Because RESUME converts itself to RESUME_CHECK, this shouldn't matter in terms of performance, so maybe keep it simple?

Copy link
Copy Markdown
Member Author

@gaogaotiantian gaogaotiantian Feb 26, 2024

Choose a reason for hiding this comment

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

Ah I remembered why I did not set this instr to RESUME_CHECK when tracing != 0 - it will convert this instr to RESUME_CHECK but RESUME_CHECK will be deopted as the version does not add up.

For your question - the key is to stop the code from being instrumented, removing this check will still result in an instrumented code, and instrumented code is always slower.

However, we should probably check if the code is tracing and do not deopt RESUME_CHECK if so.

Copy link
Copy Markdown
Member Author

@gaogaotiantian gaogaotiantian Feb 26, 2024

Choose a reason for hiding this comment

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

Another way to do this is to DISPATCH() in the instrument case, so we don't have to write the "normal" case twice. Is it frown upon to have multiple DISPATCH() in one bytecode?

Copy link
Copy Markdown
Member

@markshannon markshannon Feb 27, 2024

Choose a reason for hiding this comment

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

Multiple DISPATCH()s should be fine.

if (code_version != global_version) {
int err = _Py_Instrument(_PyFrame_GetCode(frame), tstate->interp);
ERROR_IF(err, error);
next_instr = this_instr;
} else {
this_instr->op.code = RESUME_CHECK;
}
}
else {
if (next_instr != this_instr) {
if ((oparg & RESUME_OPARG_LOCATION_MASK) < RESUME_AFTER_YIELD_FROM) {
CHECK_EVAL_BREAKER();
}
this_instr->op.code = RESUME_CHECK;
}
}

Expand All @@ -175,7 +178,7 @@ dummy_func(
inst(INSTRUMENTED_RESUME, (--)) {
uintptr_t global_version = _Py_atomic_load_uintptr_relaxed(&tstate->interp->ceval.eval_breaker) & ~_PY_EVAL_EVENTS_MASK;
uintptr_t code_version = _PyFrame_GetCode(frame)->_co_instrumentation_version;
if (code_version != global_version) {
if (code_version != global_version && tstate->tracing == 0) {
if (_Py_Instrument(_PyFrame_GetCode(frame), tstate->interp)) {
GOTO_ERROR(error);
}
Expand Down
17 changes: 10 additions & 7 deletions Python/generated_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.