[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[STINNER Victor]

STINNER Victor  added the comment:

commit 46f59ebd01e22cc6a56fd0691217318c1d801a49
Author: Christian Heimes 
Date:   Wed Nov 18 16:12:13 2020 +0100

bpo-1635741: Port _hashlib to multiphase initialization (GH-23358)

Signed-off-by: Christian Heimes 


See bpo-4 "Convert a few stdlib extensions to the limited C API (PEP 384)".

--
nosy: +vstinner
resolution:  -> duplicate
stage: patch review -> resolved
status: open -> closed
superseder:  -> Py_Finalize() doesn't clear all Python objects at exit

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Robin Schreiber]

Changes by Robin Schreiber robin.schrei...@me.com:


--
keywords: +pep3121 -patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 So I'd propose that it is actually the leaf subtype which decrefs
 ob_type. The check whether you are the leaf type is then done by
 checking whether tp_dealloc is the one you are in right now.

By leaf, you mean the derived subtype? That sounds a bit quirky (you
need the aforementioned if), how about the base heap type instead?

Speaking of which, what does this refactoring bring exactly? The type
declarations are slightly less verbose, but other than that, is there a
point? (using the stable ABI doesn't seem to provide any benefit for
standard extension modules; besides, using PyType_FromSpec is only a
small part of complying with the stable ABI)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Martin v . Löwis]

Martin v. Löwis added the comment:

 By leaf, you mean the derived subtype? That sounds a bit quirky (you
 need the aforementioned if), how about the base heap type instead?

I think this fails (currently) because a subtype defined in Python using
a class statement will automatically get subtype_dealloc as its dealloc
function, which will in turn unconditionally decrefs the type (after
calling the tp_dealloc function from its nearest ancestor which doesn't
use subtype_dealloc).

 Speaking of which, what does this refactoring bring exactly? The type
 declarations are slightly less verbose, but other than that, is there a
 point?

It's really about the PEP 3121 changes: modules shouldn't have any
global variables shared across interpreters, so that module cleanup
can work properly. Ultimately, this can lead to gc-based shutdown,
module unloading, and better separation of interpreters.

In addition, it further reduces the differences between extension
types and classes (which supposedly were removed by dropping
old-style classes, yet some inconsistencies remain where the
interpreter offers some features only to heaptypes).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Greg Stein]

Changes by Greg Stein gst...@gmail.com:


--
nosy:  -gstein

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Andrew Svetlov]

Changes by Andrew Svetlov andrew.svet...@gmail.com:


--
nosy: +asvetlov

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Martin v . Löwis]

Martin v. Löwis added the comment:

Antoine: Py_DECREF calls tp_dealloc directly, so the type needs to be DECREFed 
in the course of tp_dealloc. I don't think there is any alternative to that.

One may wonder why regular extension types don't do that: this is because of a 
hack that excludes static (non-heap) types from being reference counted in 
their instances. Heap types do refcount their types, consequently, 
subtype_dealloc also DECREFs the type.

I certainly agree that this is muddy, in particular when it comes to subtyping 
where the derived subtype calls the base tp_dealloc. In an ideal world, 
object_dealloc would decref the type, and subtypes would be required to call 
the base type's dealloc. However, I feel that this cannot be changed before 
Python 4.

So I'd propose that it is actually the leaf subtype which decrefs ob_type. The 
check whether you are the leaf type is then done by checking whether tp_dealloc 
is the one you are in right now.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Robin Schreiber]

New submission from Robin Schreiber:

Changes proposed in PEP3121 and PEP384 have now been applied to the hashopenssl 
module!

--
components: Extension Modules
files: _hashopenssl_pep3121-384_v0.patch
keywords: patch
messages: 168220
nosy: Robin.Schreiber, gstein
priority: normal
severity: normal
status: open
title: PEP 3121, 384 refactoring applied to hashopenssl module
type: resource usage
versions: Python 3.4
Added file: http://bugs.python.org/file26807/_hashopenssl_pep3121-384_v0.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Antoine Pitrou]

Antoine Pitrou added the comment:

+m = PyState_FindModule(_hashlibmodule);
+if(!m){
+m = PyModule_Create(_hashlibmodule);
+if (m == NULL)
+return NULL;
+} else {
+Py_INCREF(m);
+return m;
+}

Why is this dance needed?

+if((void *)type-tp_dealloc == (void *)EVP_dealloc) {
+Py_DECREF(type);
+}

Why?

--
nosy: +gregory.p.smith, loewis, pitrou

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Antoine Pitrou]

Changes by Antoine Pitrou pit...@free.fr:


--
stage:  - patch review

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Antoine Pitrou]

Antoine Pitrou added the comment:

Something else:

+#define _hashlibstate(o) ((_hashlibstate *)PyModule_GetState(o))

It is really bad style to #define a symbol that shadows another symbol.
Since it's a #define, I would expect to be named something like STATE(o).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Robin Schreiber]

Robin Schreiber added the comment:

Regarding the macro definition, I would be fine with changing it to 
_hashlib_state.

The dance you have found inside the Init, makes shure that the very same module 
is returned if Init is called twice or multiple times, before the Module is 
unloaded. A month back, when I created this patch, I had statements such as 
test.import.import_fresh_module(...) call the Init-method multiple times, 
before a module was unloaded. This was apparently a bug, as I can no longer 
reproduce this behavior, but at that time I thought it was the expected 
behavior :-)

The last code snipped verifies, that we only dereference the type if the 
dealloc function is not being called from inside the subtype_dealloc function. 
This is necessary because the subtype_dealloc function itself contains a decref 
of the respective type object. Without this check, we would then end up 
decrefing the type too many times.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 The last code snipped verifies, that we only dereference the type if
 the dealloc function is not being called from inside the
 subtype_dealloc function. This is necessary because the
 subtype_dealloc function itself contains a decref of the respective
 type object. Without this check, we would then end up decrefing the
 type too many times.

I still don't understand why it is required. You shouldn't have to
decref the type at all. Otherwise, it is a bug somewhere in Python
(typeobject.c perhaps).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Robin Schreiber]

Robin Schreiber added the comment:

Well, as I have changed the static type to a HeapType (as I am now using the 
stable ABI from PEP 384 for this type), we have to start perfoming proper 
reference counting with this object. This includes increfing the type in case a 
new object of that type is created, and decrefing if such an object is being 
deallocated.
As of now, I do not know of HeapTypes being excluded from refcounting.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Antoine Pitrou]

Antoine Pitrou added the comment:

Le mardi 14 août 2012 à 19:55 +, Robin Schreiber a écrit :
 As of now, I do not know of HeapTypes being excluded from refcounting.

No, but see http://bugs.python.org/issue15142
It's not obvious to me that the type should be explicitly decref'ed from the 
tp_dealloc function. This will make things more complicated for people willing 
to migrate their extensions to the stable ABI.

Also, all this is not documented at all :-(

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Robin Schreiber]

Robin Schreiber added the comment:

As subtype_dealloc decref'ed the HeapType I thought the dealloc method was the 
most appropriate place to decrement the refcount of the type.
However you still agree that these types need to be recounted properly, don't 
you? In that case, where would you place the decref of the type?
I have talked with Martin v. Löwis about this issue, and I think he was quite 
comfortable with placing the decref inside the dealloc method.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue15653] PEP 3121, 384 refactoring applied to hashopenssl module

[Antoine Pitrou]

Antoine Pitrou added the comment:

Le mardi 14 août 2012 à 20:16 +, Robin Schreiber a écrit :
 However you still agree that these types need to be recounted
 properly, don't you?

Yes, of course.

 In that case, where would you place the decref of the type?

That's a good question. Perhaps the DECREF mechanism / typeobject.c
should do it. Or perhaps PyType_FromSpec should place a stub which would
call the actual dealloc.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15653
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com