classification
Title: PEP 3121, 384 refactoring applied to datetime module
Type: resource usage Stage:
Components: Extension Modules Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Robin.Schreiber, asvetlov, belopolsky, lemburg, loewis, p-ganssle, pitrou
Priority: normal Keywords: patch, pep3121

Created on 2012-07-18 21:33 by Robin.Schreiber, last changed 2018-07-05 16:00 by p-ganssle.

Files
File name Uploaded Description Edit
_datetimemodule_pep3121-384_v0.patch Robin.Schreiber, 2012-07-18 21:33
_datetimemodule_pep3121-384_v1.patch Robin.Schreiber, 2012-08-14 18:09
_datetimemodule_pep3121-384_v2.patch Robin.Schreiber, 2012-08-15 08:50
_datetimemodule_pep3121-384_v3.patch Robin.Schreiber, 2012-12-10 10:39
Messages (11)
msg165805 - (view) Author: Robin Schreiber (Robin.Schreiber) * (Python triager) Date: 2012-07-18 21:33
Changes proposed in PEP3121 and PEP384 have now been applied to the datetime module!
msg166126 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-07-22 11:33
Too late for 3.3
msg168215 - (view) Author: Robin Schreiber (Robin.Schreiber) * (Python triager) Date: 2012-08-14 18:09
Fixed _dealloc methods. Also: Init now returns the previously initialized module if available.
msg168218 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-08-14 18:28
Add Alexander Belopolsky to nosy list as maintainer of datetime module.
msg168229 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-14 19:09
+    Py_CLEAR(_datetimemodulestate(m)->PyDateTime_DateTimeType);
+    Py_CLEAR(_datetimemodulestate(m)->PyDateTime_DeltaType);
+    Py_CLEAR(_datetimemodulestate(m)->PyDateTime_TimeType);
+    Py_CLEAR(_datetimemodulestate(m)->PyDateTime_TimeZoneType);
+    Py_CLEAR(_datetimemodulestate(m)->PyDateTime_DateType);
+    Py_CLEAR(_datetimemodulestate(m)->PyDateTime_TZInfoType);

Style nit: I would really store the module state pointer in a variable here, instead of repeating _datetimemodulestate(m) every line.
(same in other places, such as the module init function)

+PyObject* _Get_State(struct PyModuleDef*);

I'm not sure why a module should define such generic a function, and especially not without a "Py" prefix.

Besides, review comments from issue15653 apply here.
msg168266 - (view) Author: Robin Schreiber (Robin.Schreiber) * (Python triager) Date: 2012-08-15 08:50
I have now included the changes that Antoine suggested. The _Get_State was used for debugging purposes and is, as I think, no longer necessary.

However we have yet to find a solution for the decref inside the dealloc methods.
msg168334 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-15 20:36
> However we have yet to find a solution for the decref inside the dealloc methods.

Perhaps ask for advice on python-dev?
msg170154 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2012-09-10 02:46
For some reason there are no review links, so I'll review in this message.

Include/datetime.h

+typedef struct {
..
+} _datetimemodulestate;

Names exposed in public headers (datetime.h is a public header) should start with Py or _Py. Other offenders include _datetimemodule_state, _datetimemodule, _datetimemodulestate_global.

I don't think these names need to be defined in the header file at all.  

+typedef struct {
+    /* Forward declarations. */
+    PyObject *PyDateTime_DateType;
+    PyObject *PyDateTime_DateTimeType;
...

These are not forward declarations anymore.  There is no need for PyDateTime_ prefix.  Use names from  PyDateTime_CAPI struct.
msg170249 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2012-09-10 23:29
I would like to split this issue to separate PEP 3121 changes from PEP 384.  PEP 3121 state cleanup implementation is clearly an improvement "from a resource management point of view."  On the other hand, I don't see much benefit for the datetime module from using a stable ABI.  Unless I am missing something, PEP 384 is primarily benefiting 3rd party developers who distribute binary modules that should run under multiple Python versions.  ABI stability is not a concern for the stdlib modules.

On the other hand, the price for multi-version support is rather steep.  Statically allocated types are more efficient.  For example, with static types, PyDate_CheckExact() is a simple macro that expands into a dereference and a pointer comparison - a couple of instructions at the CPU level.  On the other hand, with a proposed patch, it will involve a function call to locate the module (PyState_FindModule), followed by another function call to locate the state (PyModule_GetState) and several more dereferences that may lead to cache misses and other pessimizations.

There is an important behavior change related to multiple interpreters.  Currently dates created by different interpreters have the same type.  With the proposed change they will have different types.  I don't think this is desirable.

In short, let's go in baby steps.  Let's implement PEP 3121 cleanup first and start a debate on the role of PEP 384 in stdlib.
msg177274 - (view) Author: Robin Schreiber (Robin.Schreiber) * (Python triager) Date: 2012-12-10 10:39
I have updated the patch to work again with the current version of the _datetimemodule. 

Regarding the suggestion of separating PEP3121 and PEP384. It might be true that datetime and other modules do not benefit directly from PEP 384, however it is still a fact that the stdlib modules should be seen as a set of reference modules, that are all implemented in a way that complies with the implementation fo the xxmodules.
I have talked with Martin von Löwis about this, and as far as I understood him correctly he also sees the PEP384 refactoring applied to the whole stdlib as a nessecary "signal" to other developers to refactor their modules accordingly.

Anyway I am planning to start to commit all of the open changes that I have created during my GSOC in the next few months. So a decision regarding this separation concern might be helpful. :-)
msg177277 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2012-12-10 11:48
On 10.12.2012 11:39, Robin Schreiber wrote:
> 
> Robin Schreiber added the comment:
> 
> I have updated the patch to work again with the current version of the _datetimemodule. 

Please use "_Py_" prefixes for private symbols you put in the header
files, e.g. _datetimemodulestate and the macros.

Question: What happens if PyModule_GetState() or PyState_FindModule()
raise an exception and return NULL ?

The current code will segfault in such a situation.

Thanks,
-- 
Marc-Andre Lemburg
eGenix.com
History
Date User Action Args
2018-07-05 16:00:09p-gansslesetnosy: + p-ganssle
2016-09-10 18:33:53belopolskysetassignee: belopolsky ->
versions: + Python 3.7, - Python 3.4
2012-12-10 11:48:41lemburgsetnosy: + lemburg
messages: + msg177277
2012-12-10 10:39:30Robin.Schreibersetfiles: + _datetimemodule_pep3121-384_v3.patch
keywords: + patch
messages: + msg177274
2012-11-08 13:19:59Robin.Schreibersetkeywords: + pep3121, - patch
2012-09-10 23:29:01belopolskysetnosy: + loewis
messages: + msg170249
2012-09-10 02:46:40belopolskysetmessages: + msg170154
2012-08-27 03:40:58belopolskylinkissue15787 dependencies
2012-08-27 03:29:41belopolskysetassignee: belopolsky
2012-08-15 20:36:24pitrousetmessages: + msg168334
2012-08-15 08:50:22Robin.Schreibersetfiles: + _datetimemodule_pep3121-384_v2.patch

messages: + msg168266
2012-08-14 19:09:58pitrousetnosy: + pitrou
messages: + msg168229
2012-08-14 18:28:51asvetlovsetnosy: + belopolsky
messages: + msg168218
2012-08-14 18:09:29Robin.Schreibersetfiles: + _datetimemodule_pep3121-384_v1.patch

messages: + msg168215
2012-07-22 11:33:50asvetlovsetnosy: + asvetlov

messages: + msg166126
versions: + Python 3.4, - Python 3.3
2012-07-18 21:33:52Robin.Schreibercreate