gh-101903: use _Py_EnterRecursiveCall internal API in _bisectmodule.c

[gdementen]

Feature or enhancement

I just noticed the _bisectmodule.c is the only internal module using the Py_EnterRecursiveCall and Py_LeaveRecursiveCall public API functions instead of the private ones with underscores. For both consistency and a tiny performance enhancement, I think it would make sense to use the private API instead.

Pitch

I have no idea of what I am doing this might be completely bogus 😉.

• Issue: Use _Py_EnterRecursiveCall private API in _bisectmodule.c #101903

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[OTheDev]

@gdementen Thanks for the PR. I've left some comments below but I think maybe we should wait and see how @rhettinger feels about using the internal static inline versions before moving forward?

---

First, in order to land this (although not a guarantee), you'll need to sign the contributor license agreement above :).

The changes you submitted look good to me. However, to address the build errors, it looks like modules tend to use this at the very top of their file (e.g. _randommodule.c, along with many others, as indicated bygrep; related is bpo-43974):

#ifndef Py_BUILD_CORE_BUILTIN
#  define Py_BUILD_CORE_MODULE 1
#endif

This should eliminate the build errors (at least it does for me).

[gdementen]

@OTheDev Thanks for the kind answer and pointer on how to solve the build issue. I'll continue on this probably one of the evenings of next week.

I don't understand what "rhettinger removed their request for review 9 hours ago" means. Does it mean it has no chance of being reviewed/accepted?

[OTheDev]

@gdementen Thanks for the reply!

I'm just as confused as you are ;) I think it is safe to assume that Raymond is very busy and needs to prioritize.

Regarding your chances of being accepted, I unfortunately cannot comment on that as I am not a core developer and I do not want to give any false hope.

However, I do think that if you think the change is meaningful, the best way to maximize your chances is to demonstrate a proof-of-concept that passes all CI tests so that it is easier to review for core developers. If a few weeks goes by and no one has commented on it, I would ping a core developer to ask if they would be willing to review the change. Then, the change may be rejected or accepted (with possible revisions requested).

BTW, I apologize if my comment in the issue gave any false hope. I hadn't realized that the macros I suggested (or similar) in my previous comment were not already present, which makes the overall task a little more challenging.

I don't think this change will lead to a significant performance improvement but I do like consistency - the only place where the public versions are invoked in the codebase are in the bisectmodule. I've seen PRs be accepted for consistency reasons, so there's precedent, but still no guarantees.

Whether I recommend you continue with the PR or close it as not planned, I'm sorry that I cannot say :/

[gdementen]

From what I understand from #85283, the idea is to go towards less use of the internal APIs in modules instead of more.

@vstinner : could you confirm this? I would then abandon/close this PR and related issue, instead of trying to make it work.