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.

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

GH-119866: Spill stack pointer when making "escaping" calls. #119875

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented May 31, 2024

This PR:

  • Changes the stacktop int offset field in the _PyInterpreterFrame to a stackpointer pointer.
  • Wraps (almost?) all escaping calls in the interpreter in a STORE_SP() ... LOAD_IP() pair to spill and reload the stack pointer.
  • Adds an assert to verify that any call that reenters the interpreter has spilled the stack pointer.

@markshannon
Copy link
Member Author

Benchmarking shows no change in performance.

@@ -191,6 +191,8 @@ struct _ts {
PyObject *previous_executor;

uint64_t dict_global_version;
int sp_cached; /* Only used in debug builds */
Copy link
Contributor

Choose a reason for hiding this comment

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

If only used in debug builds, should this be inside #ifdef Py_DEBUG?

Copy link
Member

Choose a reason for hiding this comment

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

I think debug builds have the same ABI compatibility as release builds these days. I know we like to pretend that PyThreadState is opaque, but this seems like it might case some nasty bugs if we do this.

@iritkatriel iritkatriel added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 4, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 2bd1339 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 4, 2024
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

LGTM.

I'd wait for the refleaks tests (or run them locally if they don't kick off).

static inline PyObject**
_PyFrame_GetStackPointer(_PyInterpreterFrame *frame)
{
PyObject **sp = frame->localsplus + frame->stacktop;
frame->stacktop = -1;
#ifndef Py_DEBUG
Copy link

Choose a reason for hiding this comment

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

Shouldn't this (and the corresponding line in the next function) be #ifdef Py_DEBUG? I don't think the assertion added to _Py_Dealloc will ever see an updated value of sp_cached otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.

@markshannon
Copy link
Member Author

markshannon commented Jun 6, 2024

Given that this will conflict horribly with #118450 and we will need to add tooling to check that all escaping calls are bounded by a save/load sp pair, I'm closing this for now.

@markshannon markshannon closed this Jun 6, 2024
@Fidget-Spinner
Copy link
Member

@markshannon quick question: would it be possible to use the cases generator to automatically insert this? As part of free threaded specializing interpreter this also has a use case.

@markshannon
Copy link
Member Author

would it be possible to use the cases generator to automatically insert this?

It might be possible, but I think it better to use the tools to verify that all escaping calls are wrapped.
Rather than SAVE_IP(); ...; LOAD_IP() we can add a macro ESCAPING_CALL(...) to make it both easier to write and to verify.

As part of free threaded specializing interpreter this also has a use case.

We can do this once #118450 is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants