classification
Title: Object Initialization does not incref Heap-allocated Types
Type: behavior Stage: patch review
Components: Extension Modules, Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Christian.Tismer, eelizondo, eric.snow, nascheme, ncoghlan, petr.viktorin, scoder, vstinner
Priority: normal Keywords: patch, patch, patch

Created on 2019-01-23 20:35 by eelizondo, last changed 2019-02-25 12:27 by eelizondo.

Pull Requests
URL Status Linked Edit
PR 11661 open eelizondo, 2019-01-23 20:40
PR 11661 open eelizondo, 2019-01-23 20:40
PR 11661 open eelizondo, 2019-01-23 20:40
Messages (20)
msg334271 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2019-01-23 20:35
Heap-allocated Types initializing instances through `PyObject_{,GC}_New{Var}` will *NOT* not have their refcnt increased. This was totally fine under the assumption that static types are immortal. However, heap-allocated types MUST participate in refcounting. Furthermore, their deallocation routine should also make sure to decrease their refcnt to provide the incref/decref pair.
msg334347 - (view) Author: Stefan Behnel (scoder) * Date: 2019-01-25 08:26
It seems right that a heap allocate object owns a reference to its (non-static) type. But the mere fact that you had to adapt stdlib code makes it obvious that this will also break existing user code out there. And such breakage is very likely to remain undetected for a while, since leaking types is unlikely to have a big enough memory impact in most application to be easily detected.

This is a tough call. Any ideas for reducing the chance of breakage or increasing the chance of detecting it in user code would help.
msg334523 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-01-29 14:36
I would very much like to see this fixed.

Yes, it can make some extension types immortal, but I believe those types were relying on buggy behavior, and need to be fixed to prevent memory leaks.

I think this is a big enough change to be mentioned in What’s New.


> Any ideas for reducing the chance of breakage or increasing the chance of detecting it in user code would help.

I'd recommend that all C extensions have leak detection as part of their test suite. I don't think we can do much better :(

> leaking types is unlikely to have a big enough memory impact in most application to be easily detected.

Indeed, it's not a big enough problem in most applications.
msg334585 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2019-01-30 17:45
Hi Petr,

Please take a look at the Github PR. What do you think that's missing to move this forward? I'd be more than happy to add more documentation/testing to it. Let me know what you think
msg334818 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-02-04 10:36
I'll allocate time this week for the review.
msg335343 - (view) Author: Stefan Behnel (scoder) * Date: 2019-02-12 18:43
Victor asked me for a review, so, well, what should I say? The intention seems right, and the patch also looks good to me.

From the top of my head, I wouldn't know any problems this would produce with Cython specifically, although it's worth testing. If we find anything, then it's hopefully easy to adapt to in a point release, which Cython users can then build their code with to support Py3.8+. That's the way it usually works with Cython.

The main problem I see is that while this change may crash in some cases (the lucky cases that are easy to detect), it will leak references in others, which can be really difficult to detect. My own biased little gut feeling still wants me to believe that the impact of this could be somewhat low. Why? Well, how many heap-allocated types with a custom "tp_dealloc()" do you expect there to be? My feeling is that most existing code still uses statically allocated types for that. CPython has a couple of examples (that the PR adapts), but IIRC, that's mostly because some core devs wanted to test-ride parts of the newer type creation C-API in the standard library (a perfectly valid reason, but that also makes it a bad example). From the little valley that I sit in, I don't see a large bunch of other usages of that API out in the wild. That doesn't mean they are not there, and there might well be some large projects that could be bitten by this change. But I'm sure it's not the majority.

So, on the one hand, any breaking change to the C-API may make users end up with little maintained projects that they depend on and that break in Py3.y and later without anyone having access to the PyPI project account to push a fix release. Very annoying situation.

On the other hand, a breaking C-API change is not the only problematic case, and people have to deal with similar situations anyway. CPython changes are really just one of many, many ways to render your code unusable.

I would suggest clear, open communication of this. It's solving a bug. It makes CPython safer. It's not difficult to adapt your code, once you know it's affected. The usual PY_VERSION_HEX guard will do it for you. I think we should risk it.
msg335348 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-12 19:02
> I would suggest clear, open communication of this. It's solving a bug. It makes CPython safer. It's not difficult to adapt your code, once you know it's affected. The usual PY_VERSION_HEX guard will do it for you. I think we should risk it.

It would be nice to give an example of code in the Porting guide (What's new in Python 3.8?).
msg335367 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2019-02-12 20:38
Thanks for the thorough feedback Stefan!

I would like to just add an extra thing to everything you already mentioned: 

This change will NEVER cause a crash. The change that we are introducing is an incref to a type object (no decrefs). Thus, there are two-ish scenarios:

1) The type object is immortal: This means that the type does not incref/decref its refcount automatically on instance creation. Adding this incref will not affect the fact that it's already immortal. An example of this is structsequences. The change that I added in the PR is to convert it to an refcounted type (instead of immortal).

2.1) The type is recounted (automatically): Achieved through the generic allocation which already increfs the type. Given that this refactors that incref, then this behavior should stay exactly the same.

2.2) The type is refcounted (manually): Achieved by not using the generic allocation and instead using `PyObject_{,GC}_New{Var}`. To properly refcount this type, a manual incref is required after object instantiation. Usually, I've seen this pattern in very carefully engineered types where a NULL is jammed into `tp_new` to make it into a non-instantiable type. Examples of this are Modules/_tkinter.c and Modules/_curses_panel.c. Anyways, adding this incref will leak this type, but will not cause a crash.
msg335368 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2019-02-12 20:45
Now, with that in mind - can you guys point me to the right thing to do now - how can we move this forward? :) 

* Should I write something up in python-dev/Discourse?
* Do I need to update the PY_VERSION_HEX?
* Do I need to write an entry in the Porting guide?

Let me know what you think!
msg335370 - (view) Author: Stefan Behnel (scoder) * Date: 2019-02-12 21:03
Adding Christian Tismer to the nosy list since he might be able to elaborate on the impact on PySide (which IIRC uses the stable ABI, and thus, heap types).
msg335419 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-13 11:47
> * Should I write something up in python-dev/Discourse?

Please open a thread on python-dev. The purpose is not really to ask if it's worth it, but more to communicate properly on backward incompatible changes.

> * Do I need to update the PY_VERSION_HEX?

No. The release manager is responsible for that.

> * Do I need to write an entry in the Porting guide?

Yes. You should add a new "Changes in the C API" section to:
https://docs.python.org/dev/whatsnew/3.8.html#porting-to-python-3-8

This is why you can show an example of dealloc using #ifdef with PY_VERSION_HEX.

Example in 3.7 doc:
https://docs.python.org/dev/whatsnew/3.7.html#changes-in-the-c-api
msg335555 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2019-02-14 19:11
> Please open a thread on python-dev
Done! https://mail.python.org/pipermail/python-dev/2019-February/156322.html

> Yes. You should add a new "Changes in the C API" 
Done as well, I also included examples for the scenarios that will need fixing to avoid having immortal types: https://github.com/python/cpython/pull/11661

Looking forward to seeing this through!
msg335592 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-02-15 08:47
Thanks for including me here!

We have indeed converted everything to new style types
but saw no refcount problems, yet. This will probably
come after the patch. We will add an issue to the tracker
for Python 3.8.
msg335695 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2019-02-16 16:40
Having types created through the stable ABI potentially be deallocated when their instances are deallocated is indeed a major problem, and fixing that seems worth the risk of some types that were designed to handle that become immortal.
msg336052 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2019-02-20 09:46
Bump to get a review on the PR: https://github.com/python/cpython/pull/11661. 

I believe all comments have now been addressed as well as adding a porting to 3.8 guide.
msg336247 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2019-02-21 18:15
Hello Eddie,
Thank you for putting what looks to be significant effort into this PR.  It would be great if we can get this fixed.  There is a real issue about breaking 3rd party extensions.  So, we want to proceed with care.

I wonder, if we are going to break extensions already, could we just remove the whole concept of heap allocated types?  If you look through the CPython source code, I think you will find a lot of tricky code that deals with the Py_TPFLAGS_HEAPTYPE case.  If we could remove heap types, we could remove all those cases.  That would give some performance improvement but more importantly would simplify the implementation.

If PyType_FromSpec() is now working correctly, could we just move everything that the currently a heap type to use that?  Obviously we have to give 3rd party extensions a lot of time to get themselves updated.  Maybe give a deprecation warning if Py_TPFLAGS_HEAPTYPE is used.  You could have a configuration option for Python that enables or disables the Py_TPFLAGS_HEAPTYPE support.  Once we think extensions have been given enough time to update themselves, we can remove Py_TPFLAGS_HEAPTYPE.

Some other possible advantages of getting rid of heap types:

- GC objects will always have the GC header allocated (because CPython controls the allocation of the chunk of memory for the type)

- might be possible to eliminate GC headers and use bitmaps.  I have been experimenting with the idea but it seems to require that we don't use heap types.  Initially I was interested in the bitmap idea because of memory savings.  After more tinkering, I think the big win will be in eliminating linked-list traversals.  On modern CPUs, that's slow and iterating over a bitmap should be much faster.

- I suspect heap types are difficult to support for PyPy.  I haven't looked into that but it seems tricky when you have non-refcounting GC

- type_is_gc() is ugly and would go away.  Based on my profiling, PyObject_IS_GC() is pretty expensive.  A lot of types have the tp_is_gc slot set (more than you would expect).

- In the very long term, using PyType_FromSpec() could give us the freedom to change the structure layout of types.  I don't have any specific ideas about that but it seems like a better design.
msg336250 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2019-02-21 18:39
Sorry, morning coffee didn't kick in yet I guess. ;-)  My actual wish is to make all types heap allocated and eliminate the statically allocated ones.  So, Py_TPFLAGS_HEAPTYPE would be set on all types in that world.  That is a gigantic task, affecting near every Python extension type.  Too huge for even a nutty person like me to imagine doing in the near term.  So, sorry for potentially derailing discussion here.

I agree with comments made by Stefan Behnel and Petr Viktorin.  There is a small risk to cause problems (i.e. serious memory leaks in a previously working program).  However, as Petr says, the extension in that case is broken and it is not hard to fix.  Eddie has provided examples for what changes are needed.

I think if we properly communicate the change then it is okay to merge the PR.
msg336286 - (view) Author: Christian Tismer (Christian.Tismer) * (Python committer) Date: 2019-02-22 08:44
Neil, that is the absolute super-move!

When all types are heap types, then I have no longer the problem
that I cannot get at slots from builtin types, with all are static.

I am very much for that change, because then I can make my stable
ABI implementation of PySide much cleaner.
msg336289 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2019-02-22 09:46
Changing all types to heap types is definitely a gigantic task. First let's make heap types more usable and bug-free, and then it will be easier :)

(I do have an agenda here: improving heap types usable is also yak-shaving* for making modules play nice with subinterpreters, like in PEPs 489 & 573)

* https://en.wiktionary.org/wiki/yak_shaving
msg336515 - (view) Author: Eddie Elizondo (eelizondo) * Date: 2019-02-25 12:27
> could we just remove the whole concept of heap allocated types?

I do have plans to start migrating more and more CPython modules to use heap allocated types. For example I have posixmodule up in the queue (PR10854) and a local change with all of _io migrated. I'm only blocked by having correct refcnts in these types.

So, yes, let's work towards migrating all the static types to heap-allocated types! I have the time and energy to embark on this huge task so you'll see more and more of these PRs from me in the future. :)

> First let's make heap types more usable and bug-free, and then it will be easier 

In that way, I agree with Petr, let's start by fixing the core issues first.



With all of that in mind, it seems to me that we are all agree on the current solution. Let's try to push this forward and merge the PR.
History
Date User Action Args
2019-02-25 12:27:40eelizondosetmessages: + msg336515
2019-02-22 09:46:31petr.viktorinsetkeywords: patch, patch, patch

messages: + msg336289
2019-02-22 08:44:51Christian.Tismersetkeywords: patch, patch, patch

messages: + msg336286
2019-02-21 18:39:48naschemesetkeywords: patch, patch, patch

messages: + msg336250
2019-02-21 18:15:48naschemesetkeywords: patch, patch, patch
nosy: + nascheme
messages: + msg336247

2019-02-20 09:46:08eelizondosetmessages: + msg336052
2019-02-16 16:40:16ncoghlansetkeywords: patch, patch, patch
nosy: + ncoghlan
messages: + msg335695

2019-02-15 22:43:18eric.snowsetkeywords: patch, patch, patch
nosy: + eric.snow
2019-02-15 08:47:32Christian.Tismersetkeywords: patch, patch, patch

messages: + msg335592
2019-02-14 19:11:17eelizondosetmessages: + msg335555
2019-02-13 11:47:18vstinnersetkeywords: patch, patch, patch

messages: + msg335419
2019-02-12 21:03:10scodersettitle: Object Initialization Bug with Heap-allocated Types -> Object Initialization does not incref Heap-allocated Types
nosy: + Christian.Tismer

messages: + msg335370

components: + Extension Modules, Interpreter Core, - Library (Lib)
type: behavior
2019-02-12 20:45:48eelizondosetmessages: + msg335368
2019-02-12 20:38:40eelizondosetmessages: + msg335367
2019-02-12 19:02:06vstinnersetkeywords: patch, patch, patch
nosy: + vstinner
messages: + msg335348

2019-02-12 18:43:19scodersetmessages: + msg335343
2019-02-04 10:36:26petr.viktorinsetkeywords: patch, patch, patch

messages: + msg334818
2019-01-30 17:45:21eelizondosetmessages: + msg334585
2019-01-29 14:36:38petr.viktorinsetkeywords: patch, patch, patch
nosy: + petr.viktorin
messages: + msg334523

2019-01-25 08:26:56scodersetnosy: + scoder
messages: + msg334347
2019-01-23 20:40:27eelizondosetkeywords: + patch
stage: patch review
pull_requests: + pull_request11463
2019-01-23 20:40:23eelizondosetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11462
2019-01-23 20:40:20eelizondosetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11461
2019-01-23 20:35:52eelizondocreate