gh-126599: Refactor the `_PyOptimizer` APIs #126853

xuantengh · 2024-11-15T03:51:24Z

Hi this is my draft to remove the _PyOptimizer_NewCounter and _PyOptimizer_NewUOpOptimizer APIs, where the related tests are removed or skipped.

For the most challenging _PyOptimizer_Optimize, I'm still trying to figure out how to achieve its functionality in generated interpreter. To remove the "artificial boundaries", should we just remove this API call and replace its code in the JUMP_BACKWARD, _EXIT_TRACE and _DYNAMIC_EXIT instruction cases?

Issue: Remove the _PyOptimizer APIs #126599

xuantengh · 2024-11-15T03:57:37Z

cc @brandtbucher @rruuaanng

rruuaanng · 2024-11-15T05:00:30Z

For the most challenging _PyOptimizer_Optimize, I'm still trying to figure out how to achieve its functionality in generated interpreter. To remove the "artificial boundaries", should we just remove this API call and replace its code in the JUMP_BACKWARD, _EXIT_TRACE and _DYNAMIC_EXIT instruction cases?

Please do not open unfinished PR. (Alternatively, you can convert it to a draft PR.)

Fidget-Spinner · 2024-11-15T10:03:08Z

Please do not open unfinished PR. (Alternatively, you can convert it to a draft PR.)

I disagree. Unfinished PRs are fine as long as they're for discussion. Also they help us lead contributors in the right direction before they put too much effort in.

rruuaanng · 2024-11-15T10:07:30Z

Please do not open unfinished PR. (Alternatively, you can convert it to a draft PR.)

I disagree. Unfinished PRs are fine as long as they're for discussion. Also they help us lead contributors in the right direction before they put too much effort in.

I don't object to this kind of behavior, but it should be opened as a draft before it is completed, which will waste review resources.
We should not open it as a PR that can be officially reviewed.

Fidget-Spinner

Thanks for picking this up. I think there might be some confusion of what is the API and what isn't so let me try my best to explain:

The API in CPython refers to what people use externally.
pycore_* files in general cannot be accessed when including the C API, unless they are included directly.
PyAPI_FUNC exports the symbol on Windows and other platforms.

So the thing we need to do is:

Make sure the API now lives in pycore_optimizer.h
Remove PyAPI_FUNC, except for when the JIT compiler needs it (The JIT compiler needs symbols exported for Windows).

So the code in JUMP_BACKWARD and ENTER_EXECUTOR shouldn't change either. We need those code for optimizing the JIT!

Lib/test/test_capi/test_opt.py

Lib/test/test_monitoring.py

Modules/_testinternalcapi.c

Python/pylifecycle.c

Fidget-Spinner · 2024-11-15T10:09:17Z

Include/internal/pycore_optimizer.h

-PyAPI_FUNC(PyObject *) _PyOptimizer_NewCounter(void);
-PyAPI_FUNC(PyObject *) _PyOptimizer_NewUOpOptimizer(void);


In theory, the only change you need is to remove the PyAPI_FUNC. That will stop the optimizer from being exported as public API.

If the header file isn't used elsewhere, we can remove it and keep the signature only in the source file.
(Sorry for the term mobile. I don't look it.)

Edit
I think. A small suggestion.

pycore_optimizer.h is used. Please don't remove it.

These 2 APIs are used for _testinternalcapi for testing. It seems like _testinternalcapi fails to be imported due to missing symbols after removing the PyAPI_FUNC.

You should likely mark them as extern if they are needed to be exported then.

IIUC, the APIs used by _testinternalcapi need to be exported? It seems extern does not help here.

Oh, right, it seems that declaring them as extern is not sufficient. So we should probably keep PyAPI_FUNC. However, you should add a comment "// Export for '_testinternalcapi' shared extension.".

picnixz · 2024-11-15T10:37:51Z

I don't object to this kind of behavior, but it should be opened as a draft before it is completed, which will waste review resources. We should not open it as a PR that can be officially reviewed.

The reason why we advised you to use draft PRs is that you are committing a lot before the code is fine to be reviewed. If a PR is in a state where it should be reviewed (for instance if the CI is green or if there is no code planned to be committed just after), then it's perfectly fine to keep it open.

rruuaanng · 2024-11-15T10:45:36Z

I don't object to this kind of behavior, but it should be opened as a draft before it is completed, which will waste review resources. We should not open it as a PR that can be officially reviewed.

The reason why we advised you to use draft PRs is that you are committing a lot before the code is fine to be reviewed. If a PR is in a state where it should be reviewed (for instance if the CI is green or if there is no code planned to be committed just after), then it's perfectly fine to keep it open.

There seems to be a little misunderstanding. I think this comment implies that there is still unfinished work :(

xuantengh · 2024-11-15T13:21:30Z

Sorry, I'm still a little confused about the goal of this PR.

Make sure the API now lives in pycore_optimizer.h

Does it mean we should define all JIT related APIs in this file?

Remove PyAPI_FUNC, except for when the JIT compiler needs it (The JIT compiler needs symbols exported for Windows).

It seems that the currently exported APIs in pycore_optimizer.h are either used by testing or JIT, so we cannot make them private. Maybe we want to hide the optimizer related APIs for users, but it seems there is not so much we can do for it?

Reverted to bring the tests back.

willingc · 2024-11-16T05:57:50Z

@xuantengh Thank you for your efforts on this PR. @Fidget-Spinner who has experience with this area of the code should be able to provide guidance as you move this PR forward.

@rruuaanng, Let's try to keep the comments positive and constructive. Please try to encourage other contributors instead of telling them what to do or not do. The goal for PRs are to build understanding during review and iterate to an optimal solution. The solution, as we have discussed before, must consider many factors including backward compatibility, maintainability, security, testing, complexity and code churn.

Sometimes a question lends much better results than telling someone what to do. For example, "Would you appreciate feedback on the PR so far?" instead of "Please do not open unfinished PR.". You can trust that the triagers and core developers are working toward creating a productive and healthy contributor experience for all. Please respect the importance of this goal.

brandtbucher · 2024-11-22T19:26:16Z

Sorry this got derailed, @xuantengh. It's also a little trickier than I originally assumed.

Let's start over. For this PR, can you just remove the "counter" optimizer, and any tests that use it? I think that's a fairly self-contained task, and gives us a better idea of what difficulty remains in extracting the other two. I also don't think it will involve deleting any files.

@rruuaanng, I can handle the review from here. I'm not worried about my time spent working with the author to get things in a good state here.

xuantengh · 2024-11-23T14:18:33Z

can you just remove the "counter" optimizer, and any tests that use it?

Sure, I'll try it.

Update: the _PyOptimizer_NewCounter API and __PyCounterOptimizer_Type type are removed. I simply skip the tests rather than deleting them.

xuantengh · 2025-01-01T03:24:11Z

I'm sorry for forgetting to remove the "draft" label for this PR.

For this PR, can you just remove the "counter" optimizer, and any tests that use it?

Regarding this goal, I think it's ready for review.

brandtbucher

Thanks for reworking this!

There's a bit more to do, though. Please also remove these:

_PyCounterOptimizerObject
counter_dealloc
_PyCounterExecutor_Type
counter_optimize
counter_get_counter
counter_optimizer_methods
Remove the check for "counting_executor" from add_executor_dependency (it isn't needed).

You'll also need to remove the _INTERNAL_INCREMENT_OPT_COUNTER bytecode instruction. It's defined in Python/bytecodes.c. Just remove it there, then run make regen-cases to update all of the generated files.

Let me know if you get stuck.

Include/internal/pycore_optimizer.h

Misc/NEWS.d/next/C_API/2025-01-01-03-25-38.gh-issue-126599.MRCYlH.rst

Lib/test/test_monitoring.py

Lib/test/test_capi/test_opt.py

bedevere-app · 2025-01-07T21:30:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

brandtbucher · 2025-01-13T22:05:03Z

@xuantengh, is this ready for review again? Happy to help if you're stuck, but I'd like to get this change in soon. :)

(Also, looks like you might need to merge in main and run make regen-all.)

…YlH.rst Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

xuantengh · 2025-01-14T01:37:09Z

I have made the requested changes; please review again.

Hi @brandtbucher, I've merged in main and rerun make regen-all. BTW, should I avoid pushing commits in the PR branch if it hasn't completed to avoid disturbing reviewers?

bedevere-app · 2025-01-14T01:37:13Z

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

brandtbucher · 2025-01-15T00:07:59Z

Hi @brandtbucher, I've merged in main and rerun make regen-all. BTW, should I avoid pushing commits in the PR branch if it hasn't completed to avoid disturbing reviewers?

It's not a problem to push new commits periodically (just don't force-push, please... merge instead of rebasing). I just wasn't sure if it was were ready yet. :)

brandtbucher · 2025-01-15T00:08:48Z

I'll try to finish reviewing this today, but I might not finish until tomorrow.

brandtbucher

Looks great, thanks! I'm just going to push a couple of changes to address some warnings and clean up the diff.

bedevere-bot · 2025-01-17T02:40:38Z

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

Hi! The buildbot aarch64 Fedora Stable LTO 3.x has failed when building commit b44ff6d.

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/#/builders/336/builds/6169) and take a look at the build logs.
Check if the failure is related to this commit (b44ff6d) 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/#/builders/336/builds/6169

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

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 996, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/test/test_interpreters/test_stress.py", line 30, in task
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 1054, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 996, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/test/test_interpreters/test_stress.py", line 47, in run
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k

xuantengh requested review from ericsnowcurrently and markshannon as code owners November 15, 2024 03:51

bedevere-app bot added the awaiting review label Nov 15, 2024

bedevere-app bot mentioned this pull request Nov 15, 2024

Remove the _PyOptimizer APIs #126599

Open

xuantengh marked this pull request as draft November 15, 2024 08:44

bedevere-app bot removed the awaiting review label Nov 15, 2024

Fidget-Spinner reviewed Nov 15, 2024

View reviewed changes

xuantengh force-pushed the remove-pyopt-api branch 2 times, most recently from b51de05 to 4c0e6c0 Compare November 23, 2024 15:56

xuantengh marked this pull request as ready for review January 1, 2025 02:58

bedevere-app bot added the awaiting review label Jan 1, 2025

xuantengh force-pushed the remove-pyopt-api branch from f6e0abb to 3f89146 Compare January 1, 2025 03:23

brandtbucher requested changes Jan 7, 2025

View reviewed changes

bedevere-app bot removed the awaiting review label Jan 7, 2025

bedevere-app bot added the awaiting changes label Jan 7, 2025

xuantengh added 2 commits January 14, 2025 09:12

remove _PyOptimizer_NewCounter

a5bd3f1

remove uop optimize tests

a77915c

xuantengh and others added 11 commits January 14, 2025 09:12

revert commits

16681b0

add export explanation

68dd2b1

remove _PyCounterOptimizer_Type and _PyOptimizer_NewCounter

f3542b6

📜🤖 Added by blurb_it.

d763008

remove _INTERNAL_INCREMENT_OPT_COUNTER

a7f9df3

remove _PyCounterOptimizerObject

44aa7ab

remove misc. stuff

0e96ecb

Update Misc/NEWS.d/next/C_API/2025-01-01-03-25-38.gh-issue-126599.MRC…

ce188a4

…YlH.rst Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

remove unnecessary comments

949352e

re-gen cases

46f0fa6

fix capi test failures

f0ffafd

xuantengh force-pushed the remove-pyopt-api branch from 03555d2 to f0ffafd Compare January 14, 2025 01:32

bedevere-app bot added awaiting change review and removed awaiting changes labels Jan 14, 2025

bedevere-app bot requested a review from brandtbucher January 14, 2025 01:37

add decorator for required API

85580ca

brandtbucher added 2 commits January 16, 2025 15:14

Clean up the diff

c3f4c6b

Fix some unused code warnings

fcb9d07

brandtbucher approved these changes Jan 16, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels Jan 16, 2025

Catch up with main

f67555f

brandtbucher merged commit b44ff6d into python:main Jan 16, 2025
57 checks passed

bedevere-app bot removed the awaiting merge label Jan 16, 2025

xuantengh deleted the remove-pyopt-api branch January 19, 2025 05:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-126599: Refactor the `_PyOptimizer` APIs #126853

gh-126599: Refactor the `_PyOptimizer` APIs #126853

xuantengh commented Nov 15, 2024 •

edited by bedevere-app bot

Loading

xuantengh commented Nov 15, 2024

rruuaanng commented Nov 15, 2024

Fidget-Spinner commented Nov 15, 2024

rruuaanng commented Nov 15, 2024

Fidget-Spinner left a comment

Fidget-Spinner Nov 15, 2024

rruuaanng Nov 15, 2024 •

edited

Loading

Fidget-Spinner Nov 15, 2024

xuantengh Nov 15, 2024

picnixz Nov 15, 2024

xuantengh Nov 15, 2024

picnixz Nov 15, 2024

picnixz commented Nov 15, 2024

rruuaanng commented Nov 15, 2024

xuantengh commented Nov 15, 2024 •

edited

Loading

willingc commented Nov 16, 2024

brandtbucher commented Nov 22, 2024

xuantengh commented Nov 23, 2024 •

edited

Loading

xuantengh commented Jan 1, 2025

brandtbucher left a comment

bedevere-app bot commented Jan 7, 2025

brandtbucher commented Jan 13, 2025

xuantengh commented Jan 14, 2025 •

edited

Loading

bedevere-app bot commented Jan 14, 2025

brandtbucher commented Jan 15, 2025

brandtbucher commented Jan 15, 2025

brandtbucher left a comment

bedevere-bot commented Jan 17, 2025

		PyAPI_FUNC(PyObject *) _PyOptimizer_NewCounter(void);
		PyAPI_FUNC(PyObject *) _PyOptimizer_NewUOpOptimizer(void);

gh-126599: Refactor the _PyOptimizer APIs #126853

gh-126599: Refactor the _PyOptimizer APIs #126853

Conversation

xuantengh commented Nov 15, 2024 • edited by bedevere-app bot Loading

xuantengh commented Nov 15, 2024

rruuaanng commented Nov 15, 2024

Fidget-Spinner commented Nov 15, 2024

rruuaanng commented Nov 15, 2024

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Fidget-Spinner Nov 15, 2024

Choose a reason for hiding this comment

rruuaanng Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Fidget-Spinner Nov 15, 2024

Choose a reason for hiding this comment

xuantengh Nov 15, 2024

Choose a reason for hiding this comment

picnixz Nov 15, 2024

Choose a reason for hiding this comment

xuantengh Nov 15, 2024

Choose a reason for hiding this comment

picnixz Nov 15, 2024

Choose a reason for hiding this comment

picnixz commented Nov 15, 2024

rruuaanng commented Nov 15, 2024

xuantengh commented Nov 15, 2024 • edited Loading

willingc commented Nov 16, 2024

brandtbucher commented Nov 22, 2024

xuantengh commented Nov 23, 2024 • edited Loading

xuantengh commented Jan 1, 2025

brandtbucher left a comment

Choose a reason for hiding this comment

bedevere-app bot commented Jan 7, 2025

brandtbucher commented Jan 13, 2025

xuantengh commented Jan 14, 2025 • edited Loading

bedevere-app bot commented Jan 14, 2025

brandtbucher commented Jan 15, 2025

brandtbucher commented Jan 15, 2025

brandtbucher left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 17, 2025

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

gh-126599: Refactor the `_PyOptimizer` APIs #126853

gh-126599: Refactor the `_PyOptimizer` APIs #126853

xuantengh commented Nov 15, 2024 •

edited by bedevere-app bot

Loading

rruuaanng Nov 15, 2024 •

edited

Loading

xuantengh commented Nov 15, 2024 •

edited

Loading

xuantengh commented Nov 23, 2024 •

edited

Loading

xuantengh commented Jan 14, 2025 •

edited

Loading