Skip to content

GH-133231: Add JIT utilities in sys._jit #133233

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented May 1, 2025

@brandtbucher brandtbucher added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-JIT labels May 1, 2025
@brandtbucher brandtbucher self-assigned this May 1, 2025
Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

quick doc-only review:

Comment on lines 1314 to 1322
Return ``True`` if the topmost Python frame is currently executing JIT
code, and ``False`` otherwise.

.. note::

Due to the nature of tracing JIT compilers, repeated calls to this
function may give surprising results. For example, branching on its
return value will likely lead to unexpected behavior (if doing so
causes JIT code to be entered or exited):
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 this section should have more emphasis that this is mainly for testing/debugging, per your rationale in Discourse:

  • It seems useful for tests for the JIT itself.
  • Lots of people have asked: “How do I know if the JIT is actually working?”

I would also suggest upgrading the 'note' about branching to a 'warning', if nothing else changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had thought that "warning" was reserved for security issues, or possible misuse of an API that could corrupt interpreter state or lead to crashes. This is just saying "the results may not be what you expect". Is a warning more appropriate than a note for calling out quirks like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

How's adding this to the top of the note:

This function is intended for testing and debugging the JIT itself. Beyond using it to confirm that the JIT is actually working, most users will never need to use it.

Comment on lines +1284 to +1290
.. envvar:: PYTHON_JIT

On builds where experimental just-in-time compilation is available, this
variable can force the JIT to be disabled (``0``) or enabled (``1``) at
interpreter startup.

.. versionadded:: 3.13
Copy link
Member

Choose a reason for hiding this comment

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

We should backport this, should it be split out into a different PR for ease?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can just backport it separately, since I need the ref for this PR's changes.

@brandtbucher
Copy link
Member Author

I just realized that my approach for sys._jit.is_active() will need reworking, since the tail-calling interpreter doesn't have entry_frame handy.

@brandtbucher
Copy link
Member Author

Okay, reworked to support tail-calling.

@brandtbucher
Copy link
Member Author

@markshannon, let me know what you think. The bots won't let us merge features without approving review, but I'd like to get something like this in before the freeze.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-JIT type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants