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 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 added 5 commits November 23, 2024 20:03

remove _PyOptimizer_NewCounter

720c99e

remove uop optimize tests

a89d0b1

remove _PyOptimizer_NewUOpOptimizer

9973709

revert commits

6a8453d

add export explanation

e4f6f46

xuantengh force-pushed the remove-pyopt-api branch from 2d913a7 to b51de05 Compare November 23, 2024 15:50

remove _PyCounterOptimizer_Type and _PyOptimizer_NewCounter

4c0e6c0

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

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

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

gh-126599: Refactor the _PyOptimizer APIs #126853

Are you sure you want to change the base?

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

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