[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Paul Ganssle]

Change by Paul Ganssle :


--
nosy: +p-ganssle

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Alexander Belopolsky]

Changes by Alexander Belopolsky :


--
assignee: belopolsky -> 
versions: +Python 3.7 -Python 3.4

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Marc-Andre Lemburg]

Marc-Andre Lemburg added the comment:

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

--
nosy: +lemburg

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Robin Schreiber]

Robin Schreiber added the comment:

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. :-)

--
keywords: +patch
Added file: 
http://bugs.python.org/file28274/_datetimemodule_pep3121-384_v3.patch

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Robin Schreiber]

Changes by Robin Schreiber :


--
keywords: +pep3121 -patch

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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.

--
nosy: +loewis

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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.

--

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Alexander Belopolsky]

Changes by Alexander Belopolsky :


--
assignee:  -> belopolsky

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Antoine Pitrou]

Antoine Pitrou added the comment:

> However we have yet to find a solution for the decref inside the dealloc 
> methods.

Perhaps ask for advice on python-dev?

--

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Robin Schreiber]

Robin Schreiber added the comment:

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.

--
Added file: 
http://bugs.python.org/file26817/_datetimemodule_pep3121-384_v2.patch

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Antoine Pitrou]

Antoine Pitrou added the comment:

+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.

--
nosy: +pitrou

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Andrew Svetlov]

Andrew Svetlov added the comment:

Add Alexander Belopolsky to nosy list as maintainer of datetime module.

--
nosy: +belopolsky

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Robin Schreiber]

Robin Schreiber added the comment:

Fixed _dealloc methods. Also: Init now returns the previously initialized 
module if available.

--
Added file: 
http://bugs.python.org/file26803/_datetimemodule_pep3121-384_v1.patch

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Andrew Svetlov]

Andrew Svetlov  added the comment:

Too late for 3.3

--
nosy: +asvetlov
versions: +Python 3.4 -Python 3.3

___
Python tracker 

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

[issue15390] PEP 3121, 384 refactoring applied to datetime module

[Robin Schreiber]

New submission from Robin Schreiber :

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

--
components: Extension Modules
files: _datetimemodule_pep3121-384_v0.patch
keywords: patch
messages: 165805
nosy: Robin.Schreiber
priority: normal
severity: normal
status: open
title: PEP 3121, 384 refactoring applied to datetime module
type: resource usage
versions: Python 3.3
Added file: 
http://bugs.python.org/file26431/_datetimemodule_pep3121-384_v0.patch

___
Python tracker 

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