GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — GH-115802: JIT "small" code for Windows by brandtbucher · Pull Request #115964 · python/cpython

Conversation

brandtbucher commented Feb 26, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

For 32-bit Windows, the small code model works fine out-of-the-box.

For 64-bit Windows, we don't have position-independent code with GOT relocations like Linux and macOS. Instead, we compile the stencils like a Py_BUILD_CORE_MODULE binary extension module, which creates a level of indirection similar to a GOT (__impl_Py_XXX holds the address of Py_XXX). We process this just like the GOT on other platforms, and everything just works.

This does require adding PyAPI_FUNC and PyAPI_DATA to some symbols in internal headers to get the correct visibility, which is why this PR touches so many files.

Looks like this makes the Windows JIT ~3-4% faster.

Issue: Improving JIT code quality #115802

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

stonebig and ssweber reacted with heart emoji

network-shark reacted with rocket emoji

All reactions

❤️ 2 reactions
🚀 1 reaction

brandtbucher and others added 10 commits

February 21, 2024 23:15


          Implement the small code model for x86_64 and aarch64 macOS and Linux

52bb3da


          blacken

81fe5ed


          Remove references to IMAGE_REL_AMD64_REL32 and clean up comments

36de1cd


          Add comment

74860c1


          Move assert back

aa53fab


          fixup

462095c


          Catch up with main

c28b7de


          Update JIT to use small code model on Windows

dfa925c


          Export a bunch of needed symbols

f7cc2fd


          Catch up with main

ee1a3bd

brandtbucher added performance

Performance or resource usage

skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels

Feb 26, 2024

brandtbucher self-assigned this

Feb 26, 2024

bedevere-app Bot mentioned this pull request

Feb 26, 2024

Improving JIT code quality #115802

Closed

8 of 13 tasks


          Feedback from (the last) PR review

6b2428d

brandtbucher marked this pull request as ready for review

February 26, 2024 18:58

brandtbucher requested review from gvanrossum and markshannon as code owners

February 26, 2024 18:58

bedevere-app Bot added the awaiting core review label

Feb 26, 2024


          Catch up with main

677fb6e

gvanrossum reviewed

Feb 26, 2024

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm sorry, I realized halfway through the review that this touches upon several large blank spaces in my brain. So this is not a very useful review. I do think that _PyOptimize_Optimize() belongs in an internal header though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Include/cpython/optimizer.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/jit/_targets.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

markshannon reviewed

Feb 27, 2024

View reviewed changes

Comment thread

Python/bytecodes.c

		conversion_func conv_fn;
		assert(oparg >= FVC_STR && oparg <= FVC_ASCII);
		conv_fn = CONVERSION_FUNCTIONS[oparg];
		conv_fn = _PyEval_ConversionFuncs[oparg];

markshannon Feb 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We could make inst(CONVERT_VALUE replicate(4) inst(CONVERT_VALUE and rely on clang removing the lookup.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

brandtbucher Feb 29, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is there a way to do that where a fifth generic version isn't generated too? Otherwise, it doesn't help, so we might as well stay with this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon reviewed

Feb 27, 2024

View reviewed changes

Comment thread

Python/jit.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

markshannon reviewed

Feb 27, 2024

View reviewed changes

markshannon left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good, I've a few comments.

As a general comment regarding JIT development:
Don't be afraid to make small changes to bytecodes.c and other interpreter files to enable the JIT to work more smoothly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher reacted with thumbs up emoji

All reactions

👍 1 reaction

markshannon reviewed

Feb 27, 2024

View reviewed changes

Comment thread

Include/internal/pycore_ceval.h

		PyAPI_DATA(const binaryfunc) _PyEval_BinaryOps[];
		PyAPI_DATA(const conversion_func) _PyEval_ConversionFuncs[];

		PyAPI_FUNC(int) _PyEval_CheckExceptStarTypeValid(PyThreadState tstate, PyObject right);

markshannon Feb 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Rather than exposing all these symbols, could we put the function pointers in a struct, and pass that to the JIT as an argument?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

brandtbucher Feb 28, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The issue isn't finding symbols when jitting the code (jit.c is linked into the main executable and can find everything just fine).

The purpose of adding PyAPI_FUNC(...) and PyAPI_DATA(...) to the declarations is so they are declared with __declspec(dllimport) when compiling the templates. This makes Clang emit indirect memory accesses on Windows (similar to position-independent code on other platforms).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

brandtbucher added 4 commits

February 28, 2024 03:49


          Make _PyOptimizer_Optimize internal

98dea1b


          Make references to registers less cryptic

774b5bb


          Merge branch 'main' into justin-relax-b

978ec78


          Fix handling of duplicate COFF symbols

ab5a9a0

brandtbucher and others added 2 commits

February 28, 2024 03:55


          Sync branches

7282eb5


          Add missing relocations (now that jump tables are enabled)

6c0db0c

brandtbucher merged commit f0df35e into python:main

Feb 29, 2024

bedevere-app Bot removed the awaiting core review label

Feb 29, 2024

bedevere-bot commented Feb 29, 2024

Copy link

Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD14 3.x has failed when building commit f0df35e.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1232/builds/1515) and take a look at the build logs.
Check if the failure is related to this commit (f0df35e) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1232/builds/1515

Summary of the results of the build (if available):

Click to see traceback logs

Traceback (most recent call last):
  File "<frozen getpath>", line 352, in <module>
ValueError: embedded null byte
Warning -- Uncaught thread exception: RuntimeError
Exception in thread Thread-60 (task):
RuntimeError: error evaluating path


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/threading.py", line 1086, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/threading.py", line 1023, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/test/test_interpreters/test_stress.py", line 29, in task
    interp = interpreters.create()
             ~~~~~~~~~~~~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(isolated=True)
         ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
RuntimeError: interpreter creation failed
k

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request

Mar 4, 2024


          pythonGH-115802: JIT "small" code for Windows (pythonGH-115964)

29f4b45

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 25, 2024


          pythonGH-115802: JIT "small" code for Windows (pythonGH-115964)

8a9dd45

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024


          pythonGH-115802: JIT "small" code for Windows (pythonGH-115964)

8991ffa

brandtbucher added the topic-JIT label

May 31, 2024

Reviewers

gvanrossum

gvanrossum left review comments

markshannon

markshannon left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

brandtbucher

Labels

interpreter-core

(Objects, Python, Grammar, and Parser dirs)

performance

Performance or resource usage

skip news topic-JIT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

brandtbucher commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markshannon Feb 27, 2024

Choose a reason for hiding this comment

Uh oh!

brandtbucher Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

markshannon Feb 27, 2024

Choose a reason for hiding this comment

Uh oh!

brandtbucher Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 29, 2024

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brandtbucher commented Feb 26, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.