GeezerVison

cc @brandtbucher @rruuaanng

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.)

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.

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.

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.

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 :(

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

1. Make sure the API now lives in pycore_optimizer.h

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

2. 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.

@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.

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.

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.

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.

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.

@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.)

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?

Thanks for making the requested changes!

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

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. :)

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

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

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

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. 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.
4. Check if the failure is related to this commit (b44ff6d) or if it is a false positive.
5. 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):

==

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