gh-148790: Defer _PyRuntime_Initialize to Py_InitializeFromConfig.#121628
gh-148790: Defer _PyRuntime_Initialize to Py_InitializeFromConfig.#121628ZeroIntensity merged 2 commits intopython:mainfrom
Conversation
AraHaan
force-pushed
the
patch-1
branch
2 times, most recently
from
47a50c5 to
c4883ab
Compare
|
Thanks for working on this, @AraHaan! I'll take a look as soon as I can. FYI, once you've created the PR (and it isn't "draft"), it's usually best if you do not rebase. Rebasing makes it harder for reviewers. For example, it's harder to know what has changed since the last time I looked at the PR. Do note that we always squash the PR commits into a single merge commit, so you don't need to worry about cluttering up the commit log with your own intermediate commits. |
I was rebasing onto current main using the website. Sorry about that. |
When I was working on my own copy of ``Py_Initialize()`` for my own embedded interpreter that: - manually sets ``sys.path`` (set to the base name of the exe since I story the python stlib in a zip file in it's win32 resource section) - Disables the default explicit ``import site`` (set to 0) - prevents the user site-packages folder from being added (set to 0), The way I did this was with a direct copy and paste of the code in ``Py_InitializeEx`` and changed a only what I needed to implement my own ``Py_Initialize`` that suited my own needs. I hated how I needed to define ``Py_BUILD_CORE_MODULE`` and include ``#include <internal/pycore_runtime.h>`` And then checked and saw that everything but the if check in ``Py_InitializeEx`` is inside of ``Py_InitializeFromConfig`` and that the if check could be replaced easily with ``Py_IsInitialized``. Because of that I submitted this change to remove the needless code duplication here since ``Py_InitializeFromConfig`` is used anyways. It also might increase performance very slightly as a result as well for free.
|
This PR is stale because it has been open for 30 days with no activity. |
| { | ||
| PyStatus status; | ||
|
|
||
| status = _PyRuntime_Initialize(); |
There was a problem hiding this comment.
Since Py_InitializeFromConfig is called in Py_InitializeEx, this can be deferred to that call & avoids needless code duplication & wasted clock cycles on startup.
AraHaan
changed the title
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
I think this makes sense. LGTM!
ZeroIntensity
added
needs backport to 3.13
|
Thanks @AraHaan for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
|
Sorry, @AraHaan and @ZeroIntensity, I could not cleanly backport this to |
|
Sorry, @AraHaan and @ZeroIntensity, I could not cleanly backport this to |
|
Okay, let's not backport this. I was going to backport to prevent conflicts, but since there are already conflicts, there's no real benefit. |
|
I think the conflicts was due to the merge commit to resolve conflicts from |
3 participants
When I was working on my own copy of
Py_Initialize()for my own embedded interpreter that:sys.path(set to the base name of the exe since I store the python stlib in a zip file in it's win32 resource section)import site(set to 0)The way I did this was with a direct copy and paste of the code in
Py_InitializeExand changed a only what I needed to implement my ownPy_Initializethat suited my own needs. I hated how I needed to definePy_BUILD_CORE_MODULEand include#include <internal/pycore_runtime.h>And then checked and saw that everything but the if check inPy_InitializeExis inside ofPy_InitializeFromConfigand that the if check could be replaced easily withPy_IsInitialized. Because of that I submitted this change to remove the needless code duplication here sincePy_InitializeFromConfigis used anyways. It also might increase performance very slightly as a result as well for free.Because this is a trivial change to an implementation detail and does not affect functionality at all, I feel like this might not need an issue first. Very glad I took the time to look at this code and seen that it could be simplified somewhat.