[issue15849] PEP 3121, 384 Refactoring applied to xx module

[STINNER Victor]

Change by STINNER Victor :


--
resolution:  -> duplicate
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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[STINNER Victor]

STINNER Victor  added the comment:

> Hum, I reopen the issue: the xx module still defines types statically and so 
> the "PEP 384" part of this issue is not fixed yet.

It's now tracked by bpo-4 "Convert a few stdlib extensions to the limited C 
API (PEP 384)".

--

___
Python tracker 

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[STINNER Victor]

STINNER Victor  added the comment:

See also bpo-40077: "Convert static types to PyType_FromSpec()".

--

___
Python tracker 

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[STINNER Victor]

STINNER Victor  added the comment:

Hum, I reopen the issue: the xx module still defines types statically and so 
the "PEP 384" part of this issue is not fixed yet.

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[STINNER Victor]

STINNER Victor  added the comment:

Fixed by:

commit d5cacbb1d9c3edc02bf0ba01702e7c06da5bc318
Author: Nick Coghlan 
Date:   Sat May 23 22:24:10 2015 +1000

PEP 489: Multi-phase extension module initialization

Known limitations of the current implementation:

- documentation changes are incomplete
- there's a reference leak I haven't tracked down yet

The leak is most visible by running:

  ./python -m test -R3:3 test_importlib

However, you can also see it by running:

  ./python -X showrefcount

Importing the array or _testmultiphase modules, and
then deleting them from both sys.modules and the local
namespace shows significant increases in the total
number of active references each cycle. By contrast,
with _testcapi (which continues to use single-phase
initialisation) the global refcounts stabilise after
a couple of cycles.

--
nosy: +vstinner
resolution:  -> fixed
stage:  -> resolved
status: open -> closed

___
Python tracker 

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Stefan Krah]

Changes by Stefan Krah stefan-use...@bytereef.org:


--
nosy:  -skrah

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

I added documentation to the components list because this in the main purpose 
of this module - to serve as a template for extension module writers.

--
assignee:  - docs@python
components: +Documentation
nosy: +docs@python

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Alexander Belopolsky]

Changes by Alexander Belopolsky alexander.belopol...@gmail.com:


--
versions: +Python 3.5 -Python 3.4

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Stefan Krah]

Stefan Krah added the comment:

I think we should perhaps leave the xxmodule as an example for
static types and create another pep-384 version that mentions
*potential* performance traps.

Of course many modules won't suffer from this, but first-time
extension writers tend to paste from the examples and should
know the alternatives if they happen to write a performance
sensitive application.

--
nosy: +skrah

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

 create another pep-384 version

+1 - and with a more descriptive name than xxmodule.c

Suggestions:

* pep384module.c
* pep384demo.c
* pep384.c
* abidemo.c

--

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Robin Schreiber]

Robin Schreiber added the comment:

Antoine, regarding that the last pending problem with the patch is related to 
the NULL checking of FindModule, I would be inclined to include your proposed 
helper API. I see that issue18710 has not been included into the trunk yet, but 
I think this is mostly due to the additional _csv patch and not because of the 
proposed API itself.
So I will upload a corrected version of the patch soon, but it will rely on 
issue18710 to be accepted beforehand...

--

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Robin Schreiber]

Robin Schreiber added the comment:

Updated patch accordingly. 
Regarding the problem in 
http://mail.python.org/pipermail/python-dev/2013-August/127862.html , it can 
indeed be solved by returning the already existing module object, if PyInit is 
called multiple times. I followed the discussion and can not make out a 
definite decision on how to handle this.
My understanding is, that up to now extension modules are only supposed to be 
initialized once per interpreter, so the check I have included in, for example 
issue15651, makes sense from this perspective. There have been reasonable 
desires (from Eli) to make the module state separate from each imported module 
within the interpreter, but this would involve more drastic changes and will be 
rather part of future Python releases.
Should we therefore (for now) make this a mandatory PEP3121 convention and 
include it into the xxmodule.c refactoring?

--
Added file: http://bugs.python.org/file31355/xxmodule_pep3121-384_v2.patch

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 Updated patch accordingly. 

I don't understand why your patch still has a module state while all
objects can be looked up in the module dict.

--

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Robin Schreiber]

Robin Schreiber added the comment:

Everything except for the Xxo_Type. But you are right. Then again, why are 
these global static variables from the start? Isn't this because xxmodule is 
some kind of dummy module that is supposed to demonstrate some coding 
conventions on how to build extension modules? 
Another possibility could be the faster lookup of the variables, which is now 
of course invalidated if we store it within the module state.

--

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 Everything except for the Xxo_Type. But you are right. Then again, why
 are these global static variables from the start? Isn't this because
 xxmodule is some kind of dummy module that is supposed to
 demonstrate some coding conventions on how to build extension
 modules? 

I don't really know what the purpose of xxmodule is. But I don't think
redundant storage is a good idea, especially with different
lifetimes :-)

--

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Robin Schreiber]

Robin Schreiber added the comment:

Updated the patch, corrected multiple syntax errors and missing INCREFS. Also 
added the comments that include the members names. I am yet undecided regarding 
the NULL-check for FindModule.

Apart from that I have tried to build some tests that prove that loading and 
unloading the module do not cause any memory leaks. This has turned up several 
problems: For one, the only possibility to check for the leaks that PEP 3121 
tries to fix, is to run PyInit of the respective module multiple times. This is 
only possible if Py_finalize() has been called after the module has been 
imported beforehand. This means we can not test for these leaks from within 
Python, but need some C-Code that calls
Py_initialize(); ... import xx ... Py_finalize(); multiple times. The problem 
is that also the plain Py_initialize(); Py_finalize(); calls leak memory. 
Unfortunately the amount of objects that are leaked also seems to vary, so 
there is no constant factor that I can subtract to determine how much the 
imported module itself leaks.
So I am kind of on a dead end here. I could upload the tests scripts that I 
have written so far, if that helps.

--
keywords: +patch
Added file: http://bugs.python.org/file31272/xxmodule_pep3121-384_v1.patch

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Antoine Pitrou]

Antoine Pitrou added the comment:

 In the example Martin gave in his PEP 3121, the PyInit does not perform any 
 INCREFs on
 the Variables that are referenced from inside the module state.
 He therefore left out m_free completely as there was nothing to DECREF within 
 the module
 state.
 Back when I did my GSoC together with Martin, we decided that the Module 
 state itself can 
 be considered a valid container object, an therefore has to INCREF and in the 
 end of its
 lifecycle (that is within m_free) also DECREF every object reference it 
 holds. I therefore 
 decided to include that into every module I refactored, and consequently also 
 the xxmodule.

I agree with that, but then PEP 3121's example code should be fixed.

 I am yet undecided regarding the NULL-check for FindModule.

Not checking for NULL makes it dangerous to implement unloading of extension 
modules: see issue18674.

If checking for NULL makes extension code too complicated, then please take a 
look at the helper API I've suggested in issue18710.

Also, it would be nice if you could also read the following python-dev thread, 
since it discusses concrete issues and possible solutions:
http://mail.python.org/pipermail/python-dev/2013-August/127862.html

 This means we can not test for these leaks from within Python, but need some 
 C-Code

You can use _testcapi.run_in_subinterp() to run custom code in a 
sub-interpreter.

Here is an example of a non-leaking extension module, and then a (presumably) 
leaking one:

$ ./python -Xshowrefcount
Python 3.4.0a1+ (default:9e61563edb67, Aug 12 2013, 14:52:25) 
[GCC 4.7.3] on linux
Type help, copyright, credits or license for more information.
 import _testcapi
[64175 refs, 19937 blocks]
 _testcapi.run_in_subinterp(import resource)
0
[68839 refs, 20969 blocks]
 _testcapi.run_in_subinterp(import resource)
0
[68852 refs, 20980 blocks]
 _testcapi.run_in_subinterp(import resource)
0
[68850 refs, 20979 blocks]
 _testcapi.run_in_subinterp(import resource)
0
[68852 refs, 20980 blocks]
 _testcapi.run_in_subinterp(import resource)
0
[68850 refs, 20979 blocks]
 _testcapi.run_in_subinterp(import _socket)
0
[71840 refs, 22059 blocks]
 _testcapi.run_in_subinterp(import _socket)
0
[71911 refs, 22083 blocks]
 _testcapi.run_in_subinterp(import _socket)
0
[71981 refs, 22107 blocks]
 _testcapi.run_in_subinterp(import _socket)
0
[72051 refs, 22131 blocks]

--
nosy: +pitrou

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Robin Schreiber]

Robin Schreiber added the comment:

I absolutely agree on mentioning the member names in the comments. :-)

In the example Martin gave in his PEP 3121, the PyInit does not perform any 
INCREFs on the Variables that are referenced from inside the module state.
He therefore left out m_free completely as there was nothing to DECREF within 
the module state.
Back when I did my GSoC together with Martin, we decided that the Module state 
itself can be considered a valid container object, an therefore has to INCREF 
and in the end of its lifecycle (that is within m_free) also DECREF every 
object reference it holds. I therefore decided to include that into every 
module I refactored, and consequently also the xxmodule.

I was also thinking about redefining the macro of xx_state_global with a NULL 
check, however this would lead either to a redundant call of PyState_FindModule 
(Which may lead to unnecessary performance degregation as xx_state_global is 
used quite frequently in some parts of the respective module) or I had to find 
some awkward way to store the result o f FindModule in some local variable 
exapnded by the macro, which I would not consider a good idea. Instead Martin 
and I were thinking of including a NULL safe variant of xx_state_global only in 
CPython Debug Builds. What do you think about that?

--

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Robin Schreiber]

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


--
keywords: +pep3121 -patch

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Robin Schreiber]

New submission from Robin Schreiber:

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

--
components: Extension Modules
files: xxmodule_pep3121-384_v0.patch
keywords: patch
messages: 169702
nosy: Robin.Schreiber, belopolsky
priority: normal
severity: normal
status: open
title: PEP 3121, 384 Refactoring applied to xx module
type: resource usage
versions: Python 3.4
Added file: http://bugs.python.org/file27098/xxmodule_pep3121-384_v0.patch

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

xxmodule.c is used as an example in PEP 3121 itself.  To the extent the recipe 
in the PEP is complete, the changes to actual xxmodule.c should follow the 
text.  For example, the text in PEP recommends to leave m_free member of 
PyModuleDef 0:

static struct PyModuleDef xxmodule = {
  {}, /* m_base */
  sizeof(struct xxstate),
  xx_methods,
  0,  /* m_reload */
  xx_traverse,
  xx_clear,
  0,  /* m_free - not needed, since all is done in m_clear */
}


In your patch, member names are not shown in the initializer.  The PEP text 
also omits them when it is obvious from the value.  (xx_clear initializes 
m_clear.)  I think all lines in the initializer should include the member name 
in comments.  The reason is that people will use xxmodule.c as a template for 
their code and may want to replace xx_clear with something that is not as 
suggestive.

You should add some tests demonstrating that module load/unload cycles do not 
introduce reference leaks.

--
nosy: +loewis

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

[issue15849] PEP 3121, 384 Refactoring applied to xx module

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

+#define xx_state_global 
+   ((xxstate *)PyModule_GetState(PyState_FindModule(xxmodule)))

This is unsafe: PyState_FindModule(xxmodule) can return NULL.  I think code 
should account for this possibility and not use this macro.  For example, 
XxoObject_Check(v) should be defined as

(PyState_FindModule(xxmodule) == NULL ? 0 : \
  (Py_TYPE(v) == xx_state(PyState_FindModule(xxmodule))-Xxo_Type))

(Should this also check PyModule_GetState() return?)

--

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