classification
Title: [subinterpreters][C API] Add a new function to create PyStructSequence from Heap.
Type: Stage: patch review
Components: C API Versions: Python 3.11
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, erlendaasland, pablogsal, petr.viktorin, rhettinger, shihai1991
Priority: normal Keywords: patch

Created on 2021-09-06 11:10 by shihai1991, last changed 2021-11-16 15:20 by petr.viktorin.

Pull Requests
URL Status Linked Edit
PR 28573 open shihai1991, 2021-09-26 17:35
Messages (24)
msg401129 - (view) Author: Hai Shi (shihai1991) * (Python triager) Date: 2021-09-06 11:10
Copied from https://bugs.python.org/issue40512#msg399847:

Victor: PyStructSequence_InitType2() is not compatible with subinterpreters: it uses static types. Moreover, it allocates tp_members memory which is not released when the type is destroyed. But I'm not sure that the type is ever destroyed, since this API is designed for static types.

> PyStructSequence_InitType2() is not compatible with subinterpreters: it uses static types. Moreover, it allocates tp_members memory which is not released when the type is destroyed. But I'm not sure that the type is ever destroyed, since this API is designed for static types.

IMO, I suggest to create a new function, PyStructSequence_FromModuleAndDesc(module, desc, flags) to create a heaptype and don't aloocates memory block for tp_members,something like 'PyType_FromModuleAndSpec()`.
msg401131 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-09-06 11:18
I think you will run into issues with allocating tp_members, because there isn't a good mechanism to for type objects to manage C-level data.

But I encourage you to try, so you get a better understanding of the problem :)
msg401132 - (view) Author: Hai Shi (shihai1991) * (Python triager) Date: 2021-09-06 11:29
> But I encourage you to try, so you get a better understanding of the problem :)

OK, thanks, Petr. I try to add this C API to see what will happen :)
msg402158 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-09-19 18:20
Is the subinterpreters work still on hold pending a PEP?

I thought that conversions to heap types had been suspended because there is a Steering Council level cost benefit decision.  Mostly it seems that everything helps subinterpreters makes code worse in almost every other way (slower, more complex, new APIs, etc).
msg402293 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-09-21 08:43
Subinterpreters are not the only reason to do this (and they're not *my* reason to do it).

Adding a way to create PyStructSequence heap will help users of the stable ABI, where reduced performance is a known tradeoff (see https://www.python.org/dev/peps/pep-0652/#stable-abi: "The Stable ABI trades performance for its stability").
More generally, this would need solving one of the remaining limitations of the limited API (PEPs 489, 630): type-specific data. If Hai Shi solves the problem, the solution will be useful even if PyStructSequence_FromModuleAndDesc turns out useless. 

Using the proposed new API in CPython's stdlib should be done much more deliberately, and yes, would need a PEP.
msg402606 - (view) Author: Hai Shi (shihai1991) * (Python triager) Date: 2021-09-25 07:23
> More generally, this would need solving one of the remaining limitations of the limited API (PEPs 489, 630): type-specific data.
Agree. We can't track and destroy the memory block created from the heap now.
msg403728 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-10-12 12:05
Oh! I assumed this bug wasn't resolved, but it is -- in bpo-34784. Sorry, I should have checked!

The only thing the proposed PR adds is a way to set ht_module, which actually isn't very useful (it's used for module state access, but PyStructSequence_Desc doesn't allow you to add custom methods or anything else that would need the module state).

I'd close this bpo as duplicate. And close the PR -- it's well done, but unfortunately, solves the wrong problem :(
msg403887 - (view) Author: Hai Shi (shihai1991) * (Python triager) Date: 2021-10-14 05:24
> The only thing the proposed PR adds is a way to set ht_module, which actually isn't very useful (it's used for module state access.

Hm, there have some static types. so I create the PyStructSequence_FromModuleAndDes() could receive tp_flags and try convert those static types to the heap type. eg: https://github.com/python/cpython/blob/main/Python/sysmodule.c#L2837-L2839
We can not do this converting operation by calling PyStructSequence_NewType(), no?

This is my *reason* to create PR-28573 :)
msg404306 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-10-19 14:02
Ah, sorry, I overlooked the flags.
This does beg the question: what else from PyType_Spec will be needed?
I guess we don't want to allow additional slots/methods. (Combining them would be hard anyway; if someone needs them they should make a subclass.) So it seems that flags and ht_module are all that's missing.


Also, note that converting the stdlib to heap types is suspended, pending a PEP. I'd be much happier with adding this if some *other* project needed it.
The Py_TPFLAGS_DISALLOW_INSTANTIATION is very specific; do you have other examples that need this?
msg404492 - (view) Author: Hai Shi (shihai1991) * (Python triager) Date: 2021-10-20 16:34
>Ah, sorry, I overlooked the flags.
 It's Okay.

>This does beg the question: what else from PyType_Spec will be needed?
I guess we don't want to allow additional slots/methods.
 +1.

>Also, note that converting the stdlib to heap types is suspended, pending a PEP.
 Can I do something for this pending PEP?

> The Py_TPFLAGS_DISALLOW_INSTANTIATION is very specific; do you have other examples that need this?
  I only find this bpo-44532 :( I guess the outside user will use it MAYBE.
msg404750 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-10-22 11:07
> Can I do something for this pending PEP?

Ask Victor, he should know more. But as far as I know, no one started on it yet.


> I guess the outside user will use it MAYBE.

Py_TPFLAGS_DISALLOW_INSTANTIATION is not part of the limited API, so it would be nice to find another use case.
msg404753 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-10-22 11:14
FYI, see bpo-43916 regarding the introduction and rationale of introducing Py_TPFLAGS_DISALLOW_INSTANTIATION.

Slightly related: bpo-43908
msg404756 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-10-22 11:44
>> Can I do something for this pending PEP?
>
> Ask Victor, he should know more. But as far as I know, no one started on it yet.

Quoting PEP 630 (active PEP):

  Whenever this PEP mentions extension modules, the advice also applies to
  built-in modules, such as the C parts of the standard library. The standard
  library is expected to switch to per-module state early.


Why is not PEP 630 enough? (I find these "political" discussions a bit hard to follow sometimes :)
msg404787 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-10-22 16:15
> Why is not PEP 630 enough?

My understanding is that this entire class of code changes has been put on hold pending Steering Council approval.  It represents a great deal of code churn and creation on new APIs.  Several of the patches had had negative performance impacts and AFAICT all of the patches have made the code more complicated and harder to maintain.  Several that I looked at had no tests at all, so there was no discernible or provabler user benefit.  Also the code being affected is typically some of our most stable code that hasn't had to be changed in over a decade.  IIRC one or more of the patches created new bugs, were destabilizing, or altered the APIs in subtle ways.

Please get explicit approval from the SC before continuing to sweep through the code base, creating heap types everywhere.  Given that some essential third party modules are not going down this path, it is unlikely that general users would ever see any benefit.
msg404795 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-10-22 16:58
> My understanding is that this entire class of code changes has been put on hold pending Steering Council approval.

Can you please point me to the official SC statement regarding this? I cannot find it.

> It represents a great deal of code churn and creation on new APIs.

You cannot implement [the accepted] PEP 630 without a little bit of code churn ;) Luckily, (most of) the APIs needed are defined in PEP 573 (also accepted, now in final state).

> Several of the patches had had negative performance impacts [...]

Can you provide a list of the patches/bpo's with still unresolved performance impacts?

> [...] all of the patches have made the code more complicated and harder to maintain

I have not seen other complaints about this? Can you provide a link to a bpo or a Discourse topic? If you find the new APIs hard to use or understand, we can of course improve the documentation and the dev guide :)

> Several that I looked at had no tests at all [...]

Actually, we've added tests to heap types with Py_TPFLAGS_IMMUTABLE_TYPE and Py_TPFLAGS_DISALLOW_INSTANTIATION. Apart from that; if a type lives on the stack or on the heap should be transparent for the user, right? The existing test suite will help verify exactly that.

> [...] so there was no discernible or provabler user benefit.

The benefits are described in PEP 630 ;)

> IIRC one or more of the patches created new bugs, were destabilizing, or altered the APIs in subtle ways.

There has been cases with ref-leaks, but that happens from time to time in all types of PRs, not just heap type PRs. Those have all been caught and fixed, thanks to reviewers and ref-leak-bots.

I'm not sure what you mean with 'destabilizing'? Can you point to a specific example or issue?

Regarding APIs, there was a long discussion about immutability and disallowing instantiation during the 3.10 beta period. Those issues were fixed and unblocked.

> Please get explicit approval from the SC before continuing to sweep through the code base, creating heap types everywhere.

Strictly speaking, the fact that PEP 630 is accepted and active _is_ an explicit approval. It has not been withdrawn or rejected.

> Given that some essential third party modules are not going down this path [...]

Which ones? Can you provide a link?

> [...] it is unlikely that general users would ever see any benefit.

The benefits are explained in PEP 630. It is a well written PEP; I suggest you re-read it :)
msg404863 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-10-23 10:00
Pep 640 is Informational, and per PEP 1:

Informational PEPs do not necessarily represent a Python community consensus or recommendation, so users and implementers are free to ignore Informational PEPs or follow their advice.

(Will reply more tomorrow, I'm on my phone now)
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
msg404865 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-10-23 10:22
Thanks, Petr. Yes, that is correct. PEP 630 (I assume you meant 630, not 640) still describes the _rationale_ very well. I guess what's needed is a Standards Track PEP based on PEP 630 (IIUC).
msg404866 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-10-23 11:00
> Quoting PEP 630 (active PEP):
>
>   Whenever this PEP mentions extension modules, the advice also applies to
>   built-in modules, such as the C parts of the standard library. The standard
>  library is expected to switch to per-module state early.

Maybe I should remove that part, I didn't expect applying this to stdlib would be so fast/rushed.

> Can you please point me to the official SC statement regarding this? I cannot find it.

Hmm, the best I can find is https://github.com/python/steering-council/blob/main/updates/2021-02-steering-council-update.md#february-08
It asks for no changes in 3.10 beta (no longer relevant) and a PEP -- presumably one specifically for the stdlib; PEP 630 already existed.

PEP 630 works best for new code; applying it to stdlib needs additional considerations, e.g. performance impact analysis, and preserving immutability or (un)pickleability, which certainly wasn't always tested well and caused much trouble late tn the release cycle.

> Please get explicit approval from the SC before continuing to sweep through the code base, creating heap types everywhere.

That would be nice (but myself, I won't be championing that effort any time soon).

> Given that some essential third party modules are not going down this path

OTOH, others (like cryptography & Qt) are going down this path, so I'll continue improving the support.
msg404869 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2021-10-23 12:45
> Given that some essential third party modules are not going down this path, it is unlikely that general users would ever see any benefit.

I disagree with your statement. It does not reflect my experience, too.

Heap types are a prerequisite for abi3 wheels. In order to get abi3 binary wheels, extensions must use the limited API, which means they must use heap types. Stable ABI wheels are a great benefit for extension authors, because it reduces their build matrix substantially. For instance cryptography's stable abi3 wheels work with any Python version >=3.6,<4.

Heap types and stable abi3 are also a major benefit for general users. Once Cython and the NumPy stack supports limited API and stable ABI, users no longer have to wait on builds for a new Python release. Every new minor release we get several bug reports on BPO about NumPy not working on latest release. This will go away.
msg404872 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-10-23 13:32
Petr:
> Hmm, the best I can find is https://github.com/python/steering-council/blob/main/updates/2021-02-steering-council-update.md#february-08

Perfect, thanks!


Christian: Thanks for your input. If this was a discussion on Discourse, I'd press the L key on your message :)
msg404875 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-10-23 13:48
Oh, I found PEP 3121 (Extension Module Initialization and Finalization). It is a Standards Track PEP and it is accepted.

The abstract is pretty short. Let me just repost it here, for convenience:

  Extension module initialization currently has a few deficiencies. There is
  no cleanup for modules, the entry point name might give naming conflicts,
  the entry functions don't follow the usual calling convention, and multiple
  interpreters are not supported well. This PEP addresses these issues.


Quoting from the Specification section of that PEP:

  The initialization routine will be invoked once per interpreter, when the
  module is imported. It should return a new module object each time.

  In order to store per-module state in C variables, each module object will
  contain a block of memory that is interpreted only by the module. The amount
  of memory used for the module is specified at the point of creation of the
  module.

  [...]

  As all Python objects should be controlled through the Python memory
  management, usage of "static" type objects is discouraged, unless the type
  object itself has no memory-managed state.


PEP 3121 is not withdrawn; PEP 630 is not withdrawn. What is then expected of a new PEP? Or the other way around: what is missing from those two PEPs? AFAICT, PEP wise, everything is arranged and ready. Perhaps the SC can chime in and explain why another PEP is required? :)


BTW, the SC has actually asked for another _Informational_ PEP, not a Standards Track PEP, so I guess a new PEP will be very similar to PEP 630?
msg404892 - (view) Author: Hai Shi (shihai1991) * (Python triager) Date: 2021-10-23 16:16
> PEP 3121 is not withdrawn; PEP 630 is not withdrawn. What is then expected of a new PEP? Or the other way around: what is missing from those two PEPs?

Thanks Erlend for the relative information you provided. AFAIK, the stdlib is not the extension module strictly :)
msg405043 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-10-26 14:05
PEP 630 has motivations and technical notes. What needs to be documented better is how both applies to stdlib.
Specifically:
- list the behavior changes when static types are converted to heap types (mutability, pickleability, any more?), and document how to undo them when the new behavior is not better
- explain performance impacts, how to measure and analyze this
- what to watch out for and test

Whether that should be a new PEP or added to 630 doesn't matter much, IMO.

Then, any introduction of PEP 630 should be done deliberately, analyzing the pros and cons for each module separately. It should not be a mass change, and it should involve explaining the motivations & specific implications to module maintainers.

Since this is out of scope in this issue (and any other one on bpo, as far as I know), I invite discussion over at https://github.com/encukou/abi3/issues/21
msg406408 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-11-16 15:20
Back to this issue -- do we have any use case other than setting the internal Py_TPFLAGS_DISALLOW_INSTANTIATION flag?

If not, I'd like to close this (with apologies for not doing my research and letting Hai Shi do unmerged work).
If a use case is found, I suspect it'll need a different solution – perhaps allowing PyType_Slot·s.
History
Date User Action Args
2021-11-16 15:20:56petr.viktorinsetmessages: + msg406408
2021-10-26 14:05:49petr.viktorinsetmessages: + msg405043
2021-10-23 16:16:22shihai1991setmessages: + msg404892
2021-10-23 13:48:35erlendaaslandsetmessages: + msg404875
2021-10-23 13:32:14erlendaaslandsetmessages: + msg404872
2021-10-23 12:45:24christian.heimessetnosy: + christian.heimes
messages: + msg404869
2021-10-23 11:00:18petr.viktorinsetmessages: + msg404866
2021-10-23 10:22:24erlendaaslandsetmessages: + msg404865
2021-10-23 10:00:17petr.viktorinsetmessages: + msg404863
2021-10-22 16:58:50erlendaaslandsetmessages: + msg404795
2021-10-22 16:15:50rhettingersetnosy: + pablogsal
messages: + msg404787
2021-10-22 11:44:58erlendaaslandsetmessages: + msg404756
2021-10-22 11:14:00erlendaaslandsetmessages: + msg404753
2021-10-22 11:07:53petr.viktorinsetmessages: + msg404750
2021-10-20 16:34:15shihai1991setmessages: + msg404492
2021-10-19 14:02:52petr.viktorinsetmessages: + msg404306
2021-10-14 05:24:07shihai1991setmessages: + msg403887
2021-10-12 12:05:30petr.viktorinsetmessages: + msg403728
2021-09-26 17:35:26shihai1991setkeywords: + patch
stage: patch review
pull_requests: + pull_request26955
2021-09-25 07:23:46shihai1991setmessages: + msg402606
2021-09-21 08:43:17petr.viktorinsetmessages: + msg402293
2021-09-19 18:20:41rhettingersetnosy: + rhettinger
messages: + msg402158
2021-09-13 09:28:57erlendaaslandsetnosy: + erlendaasland
2021-09-06 16:39:29vstinnersetnosy: - vstinner
2021-09-06 11:29:31shihai1991setmessages: + msg401132
2021-09-06 11:18:09petr.viktorinsetmessages: + msg401131
2021-09-06 11:10:48shihai1991create