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 — bpo-47215: Add undocumented, unstable FrameStack API for use by greenlets and similar libraries. by markshannon · Pull Request #32303 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Apr 4, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Skipping news, as I want to keep this as low key as possible.
We want this for greenlets, we don't want it misused by other third party code.

@brandtbucher would this cover greenlet's needs?

https://bugs.python.org/issue47215

Sorry, something went wrong.

Uh oh!

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

All reactions
markshannon added 2 commits April 4, 2022 12:48
@markshannon
Add undocumented, unstable FrameStack API for use by greenlets and si…
5422b53
…milar libraries.
@markshannon
Add minimal documentation.
f6a46a6
@markshannon
markshannon requested a review from brandtbucher April 4, 2022 12:09
@bedevere-bot bedevere-bot added the awaiting core review label Apr 4, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 4, 2022
@markshannon markshannon added the skip news label Apr 4, 2022
jdahlin
jdahlin reviewed Apr 4, 2022
View reviewed changes
Comment thread Include/cpython/pystate.h
PyAPI_FUNC(int) _PyCrossInterpreterData_RegisterClass(PyTypeObject *, crossinterpdatafunc);
PyAPI_FUNC(crossinterpdatafunc) _PyCrossInterpreterData_Lookup(PyObject *);

/* UNSTABLE API for stackful coroutines.

@jdahlin jdahlin Apr 4, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about hiding this behind an ifdef, it would technically be an api break, but would make the intention clearer:

#ifdef _PYTHON_C_UNSTABLE_FRAMESTACK_API
...
#endif

Sorry, something went wrong.

Uh oh!

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

All reactions
@brandtbucher

brandtbucher commented Apr 5, 2022

Copy link
Copy Markdown
Member

@brandtbucher would this cover greenlet's needs?

Sorry, I didn't get a chance to look at this too closely today. My plan is to attempt to modify my fork of Greenlet tomorrow to see how well it works with this branch (and then we'll have a patch we can upstream right away, too).

A few thoughts just from looking at this so far:

  • I don't see much value in exposing a tunable chunk_size parameter, even internally.
  • As I understand it, when it comes to the Python stack, Greenlet's needs are essentially limited to saving and restoring the state of an arbitrary thread.
  • These steps also happen at different points: saving happens here, restoring happens here.
  • To that end, I'm not sure if this init/swap/free model is a very good fit. A more useful API might be (roughly):
    • void *_PyThreadState_FrameStackSave(PyThreadState *tstate)
    • void _PyThreadState_FrameStackRestore(PyThreadState *tstate, void *saved)

We can also loop in the Greenlet maintainers and get their opinion on this. @jamadden?

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Apr 7, 2022

Copy link
Copy Markdown
Member Author

I don't see much value in exposing a tunable chunk_size parameter

The default size is 64k, which is fine for a thread, but quite large for a coroutine. It costs nothing to expose it.

The problem with a save/restore model is that it has too many states and error conditions. What state is the VM in after mutliple saves, with no restore? Or vice versa?

For performance reasons the frame stack struct needs to be embedded in the thread state, so we can't return a pointer to it.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@markshannon
Merge branch 'main' into frame-stack-api
ca6b767
@markshannon markshannon mentioned this pull request Sep 5, 2022
undefined behavior: tstate->datastack_top == NULL #96569
Closed
markshannon added 2 commits September 7, 2022 13:29
@markshannon
Merge branch 'main' into frame-stack-api
b21d198
@markshannon
Change top/limit pair to free/limit pair to allow fast checks and NUL…
1a82683
…L pointer.
@brandtbucher
brandtbucher removed their request for review May 10, 2023 22:02
@markshannon markshannon mentioned this pull request Apr 10, 2022
Add "unstable" frame stack api #91371
Open
@markshannon markshannon closed this Aug 6, 2024
@markshannon
markshannon deleted the frame-stack-api branch August 6, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@jdahlin jdahlin jdahlin left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review skip news

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.

6 participants

@markshannon @brandtbucher @jdahlin @the-knights-who-say-ni @ezio-melotti @bedevere-bot
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.