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-115999: Disable the specializing adaptive interpreter in free-threaded builds #116013

Merged
merged 7 commits into from
Mar 1, 2024

Conversation

swtaarrs
Copy link
Member

For now, disable all specialization when the GIL might be disabled. We can reenable functionality as it is properly supported.

Disabling specialization was just a few lines in pycore_code.h, then there are two related changes:

  • Define an alternate implementation of DECREMENT_ADAPTIVE_COUNTER() for free-threaded builds that aborts, as a sanity check.

  • Add some code to the interpreter generator to avoid unused variable warnings for variables only used by code within #ifdef ENABLE_SPECIALIZATION blocks (like this_instr and counter here). There are a variety of ways to suppress these warnings. I went with (void)foo; because it seemed like the most straightforward option, but I'm open to other suggestions. (Py_UNUSED(foo) won't work since it's intended for variables that are completely unused, and it changes the variable's name to _unused_foo.)

  • Issue: Make the specializing interpreter thread-safe in --disable-gil builds #115999

@swtaarrs swtaarrs marked this pull request as ready for review February 29, 2024 03:35
@swtaarrs swtaarrs requested a review from colesbury February 29, 2024 03:35
@colesbury
Copy link
Contributor

!buildbot nogil

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @colesbury for commit db4f433 🤖

The command will test the builders whose names match following regular expression: nogil

The builders matched are:

  • x86-64 MacOS Intel ASAN NoGIL PR
  • x86-64 MacOS Intel NoGIL PR
  • ARM64 MacOS M1 Refleaks NoGIL PR
  • ARM64 MacOS M1 NoGIL PR
  • AMD64 Ubuntu NoGIL Refleaks PR
  • AMD64 Ubuntu NoGIL PR
  • AMD64 Windows Server 2022 NoGIL PR

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'd like someone working on the specializing interpreter to review it as well.

@brandtbucher, I saw your comment #115999. Would you be able to review this?

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I'll do the review. Looks good to me too. I was wondering if DECREMENT_ADAPTIVE_COUNTER() shouldn't just be an error. Otherwise only 1-2 nits.

Python/ceval_macros.h Outdated Show resolved Hide resolved
Lib/test/test_monitoring.py Show resolved Hide resolved
Python/ceval_macros.h Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

Let me know if you need help figuring out the remaining test failures.

@swtaarrs
Copy link
Member Author

The test_interpreters refleak failure happens intermittently on main with a normal build for me. I'll see what I can figure out.

@colesbury
Copy link
Contributor

@swtaarrs - it looks like the free-threaded refleaks builds are broken on main:

https://buildbot.python.org/all/#/builders/1226
https://buildbot.python.org/all/#/builders/1368

@swtaarrs
Copy link
Member Author

Yeah, I've been trying (unsuccessfully) to bisect the failure for a bit. I'll update this PR with Guido's comment soon either way.

@swtaarrs
Copy link
Member Author

This should be ready for review again. The test failures are from gh-116099.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Yeah! After the other PR lands you can merge the new main and then maybe @colesbury can merge this? (Or set up auto-merge.)

@colesbury colesbury merged commit 339c8e1 into python:main Mar 1, 2024
41 checks passed
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
…e-threaded builds (python#116013)

For now, disable all specialization when the GIL might be disabled.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…e-threaded builds (python#116013)

For now, disable all specialization when the GIL might be disabled.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…e-threaded builds (python#116013)

For now, disable all specialization when the GIL might be disabled.
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.

4 participants