GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-101903: use _Py_EnterRecursiveCall internal API in _bisectmodule.c by gdementen · Pull Request #101931 · python/cpython

Conversation

@gdementen

@gdementen gdementen commented Feb 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gdementen
Use _Py_EnterRecursiveCall internal API in _bisectmodule.c
1a4509b
@gdementen
gdementen requested a review from rhettinger as a code owner February 15, 2023 09:48
@bedevere-bot bedevere-bot added the awaiting review label Feb 15, 2023
@bedevere-bot bedevere-bot mentioned this pull request Feb 15, 2023
Use _Py_EnterRecursiveCall private API in _bisectmodule.c #101903
Closed
@bedevere-bot

bedevere-bot commented Feb 15, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

ghost commented Feb 15, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@arhadthedev arhadthedev added the extension-modules C modules in the Modules dir label Feb 15, 2023
@OTheDev

OTheDev commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhettinger
rhettinger removed their request for review February 17, 2023 02:40
@gdementen

gdementen commented Feb 17, 2023

Copy link
Copy Markdown
Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@OTheDev

OTheDev commented Feb 19, 2023

Copy link
Copy Markdown
Contributor

@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 :/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gdementen

gdementen commented Feb 19, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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.

OTheDev reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland closed this Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review extension-modules C modules in the Modules dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@gdementen @bedevere-bot @OTheDev @arhadthedev @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.