Skip to content
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

Jump to bottom

GH-108976. Keep monitoring data structures valid during de-optimization during callback. #109131

Merged
merged 2 commits into from
Sep 11, 2023
Merged

GH-108976. Keep monitoring data structures valid during de-optimization during callback. #109131

merged 2 commits into from
Sep 11, 2023

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Sep 8, 2023 •
edited by iritkatriel
Loading

@markshannon
Make sure that monitoring data structures remain valid during de-opti…
fe72832 
…mization in the middle of executing instrumented instructions
@markshannon
Add news
1b73f79
@bedevere-bot bedevere-bot mentioned this pull request Sep 8, 2023
Closed
@markshannon markshannon added the needs backport to 3.12 only security fixes label Sep 8, 2023
@markshannon
Copy link
Member Author

@gaogaotiantian I've copied the tests from #109043
Thanks for those, and the triaging. Much appreciated.

This PR does three things:

  • Updates and fixes the validation code. This was key to doing the actual fix.
  • Fetches the next opcode from the original_opcode table late, in case it gets changed.
  • Makes sure that the original_opcode is updated to the underlying instruction when removing per-instruction instrumentation

@gaogaotiantian
Copy link
Member

gaogaotiantian commented Sep 8, 2023 •
edited
Loading

Ah, I knew there should be a better place to fix the code, just did not know where. Yeah putting it in de_instrument_line makes more sense.

I do think _PyCoLineInstrumentationData *lines = &code->_co_monitoring->lines[i]; should be _PyCoLineInstrumentationData *line = &code->_co_monitoring->lines[i]; The variable name lines confused me a couple of times. But that's a nit.

Feel free to close my PR when this is merged :)

iritkatriel
iritkatriel reviewed Sep 11, 2023
View reviewed changes
Python/instrumentation.c
@@ -458,6 +434,9 @@ dump_instrumentation_data(PyCodeObject *code, int star, FILE*out)
static bool
valid_opcode(int opcode)
{
if (opcode == INSTRUMENTED_LINE) {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to make the generator add INSTRUMENTED_LINE to _PyOpcode_opcode_metadata

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be, but this PR needs backporting to 3.12 and it's already quite large for a backport this late in the release cycle.

Python/instrumentation.c
@@ -642,10 +633,11 @@ de_instrument_per_instruction(PyCodeObject *code, int i)
int original_opcode = code->_co_monitoring->per_instruction_opcodes[i];
CHECK(original_opcode != 0);
CHECK(original_opcode == _PyOpcode_Deopt[original_opcode]);
instr->op.code = original_opcode;
*opcode_ptr = original_opcode;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to do this through a pointer?

Copy link
Member Author

@markshannon markshannon Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we have INSTRUMENTED_LINE -> INSTRUMENTED_INSTRUCTION -> NORMAL_OPCODE,
then we want to change the original_opcode for INSTRUMENTED_LINE, not the original instruction.
That way we end up with INSTRUMENTED_LINE -> NORMAL_OPCODE, not just NORMAL_OPCODE.

This fixes #109156 as well, I think.

@markshannon markshannon mentioned this pull request Sep 11, 2023
Merged
iritkatriel
iritkatriel approved these changes Sep 11, 2023
View reviewed changes
Python/instrumentation.c
@@ -1216,7 +1208,9 @@ _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame,
}
} while (tools);
Py_DECREF(line_obj);
uint8_t original_opcode;
Copy link
Member

Choose a reason for hiding this comment

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

why not inline the declaration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because C doesn't allow a declaration immediately following a label.
For arcane historical reasons, probably.

@markshannon markshannon merged commit 4a69301 into python:main Sep 11, 2023
@miss-islington
Copy link
Contributor

Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @markshannon, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 4a69301ea4539da172a00a80e78c07e9b41c1f8e 3.12

@markshannon markshannon mentioned this pull request Sep 11, 2023
Closed
markshannon added a commit to faster-cpython/cpython that referenced this pull request Sep 11, 2023
@markshannon
pythonGH-108976. Keep monitoring data structures valid during de-opti…
646b7ce 
…mization during callback. (pythonGH-109131)
@bedevere-app
Copy link

bedevere-app bot commented Sep 11, 2023

GH-109268 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Sep 11, 2023
gaogaotiantian
gaogaotiantian reviewed Sep 11, 2023
View reviewed changes
Python/instrumentation.c
@@ -642,10 +633,11 @@ de_instrument_per_instruction(PyCodeObject *code, int i)
int original_opcode = code->_co_monitoring->per_instruction_opcodes[i];
CHECK(original_opcode != 0);
CHECK(original_opcode == _PyOpcode_Deopt[original_opcode]);
instr->op.code = original_opcode;
*opcode_ptr = original_opcode;
if (_PyOpcode_Caches[original_opcode]) {
instr[1].cache = adaptive_counter_warmup();
Copy link
Member

Choose a reason for hiding this comment

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

The cache would be initialized here even if the instr is INSTRUMENTED_LINE (but the original_opcode is an instruction with cache). I guess that's no harm?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed harmless.

Yhg1s pushed a commit that referenced this pull request Sep 12, 2023
@markshannon
[3.12] GH-108976. Keep monitoring data structures valid during de-opt…
3efe7bc 
…imization during callback. (GH-109131) (#109268)

GH-108976. Keep monitoring data structures valid during de-optimization during callback. (GH-109131)
@vstinner vstinner mentioned this pull request Sep 13, 2023
Closed
@markshannon markshannon deleted the fix-instrumented-instruction branch September 26, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaogaotiantian gaogaotiantian gaogaotiantian left review comments

@iritkatriel iritkatriel iritkatriel approved these changes

Assignees

@markshannon markshannon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@markshannon @gaogaotiantian @miss-islington @iritkatriel @bedevere-bot