classification
Title: Speed up _decimal import
Type: performance Stage: resolved
Components: Extension Modules Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: skrah Nosy List: Arfrever, barry, eric.smith, fijall, mark.dickinson, pitrou, python-dev, rhettinger, skrah, vstinner
Priority: normal Keywords: patch

Created on 2013-10-12 12:03 by skrah, last changed 2014-10-12 11:31 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue19232.patch skrah, 2013-10-12 12:48
issue19232-2.patch skrah, 2013-10-12 18:47 patch without --git option review
Messages (21)
msg199553 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-12 12:03
As discussed on python-dev, importing _decimal at the bottom of
decimal.py is about 9x slower than importing _decimal directly.
msg199555 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-12 12:21
I proposed something similar for issue #19229.
msg199558 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2013-10-12 12:43
Remember that one reason for importing the C version at the bottom of the python version is so that alternate implementations (PyPy, IronPython, Jython) could provide partial versions of the C (or equivalent) versions. By importing after the Python version, the alternate implementation could continue to use parts of the Python code.

I think the impact on alternate implementations needs to be considered before we start rearchitecting these imports.
msg199562 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-12 13:00
Right, let's start collecting objections. :)

Mark, Raymond: Would you support the change (__name__ hack and all)?


Maciej: Is this approach a problem for PyPy?
msg199564 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-12 13:08
If the Python implementation is renamed to _pydecimal, I don't expect it to be used in CPython. I never used _pyio in a real application, only for some tests to debug. I don't think that we need the __name__ = 'decimal' "hack". If you really want to keep it, please add at least a comment explaining it.
msg199567 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-12 13:20
I guess if some of the pickling stuff get's rewritten, we can drop
__name__.

The other thing is that traditionally the types were "decimal.Decimal"
etc., so I'm not sure if it is good idea to have "_decimal.Decimal" and
"_pydecimal.Decimal".

Of course adding __module__ everywhere is another option.
msg199570 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-12 13:24
> The other thing is that traditionally the types were "decimal.Decimal"
> etc., so I'm not sure if it is good idea to have "_decimal.Decimal" and
> "_pydecimal.Decimal".

Why not renaming the _decimal module to decimal?
msg199571 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-12 13:32
_decimal already lies about its name (for pickling).
msg199612 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-12 18:47
I can't apply the patch that was created with diff --git, so here is
another one that is less readable but applies.
msg199619 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-12 20:34
> I can't apply the patch that was created with diff --git, so here is
> another one that is less readable but applies.

You can apply it using "hg import --no-commit", I think.
msg199677 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-10-13 08:59
> Mark, Raymond: Would you support the change (__name__ hack and all)?

No objections here.
msg199691 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-13 10:59
Antoine Pitrou <report@bugs.python.org> wrote:
> You can apply it using "hg import --no-commit", I think.

mercurial 2.1 throws an exception even though the patch was created with
that version.  Now I upgraded to 2.7.2 and it works.

Rietveld also seems to choke on the first patch (no review link).
msg199724 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-10-13 15:35
I think we should save these sort of tricks only for modules imported during startup.

Ideally, a user should expect that the code for the decimal module is in decimal.py.  Ideally, tools like IDLE's "Open Module" should be able to find the source code using only the module name.

I'm -0 on this one.  I don't think hacking up the source tree is worth it.  The import is only done once -- the important part is what decimal does when it is imported.
msg199864 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-10-14 11:00
About IDLE I can't say anything, but I'm not entirely sure if the
PEP-399 import method is easier to understand for users, see e.g.:

http://stackoverflow.com/questions/13194384/instantiate-decimal-class

I get the impression that the posters at first did not even realize
that there *is* an accelerator in Python 3.3, so they had a hard time
debugging the issue.


As for the load time: Personally I don't have any application
that relies on a very short load time, but I wan't to note again
that importing decimal at Python startup actually doubles the
startup time.

So if there are applications that start a new Python script
for each request (e.g. via djb's tcpserver), it could really
matter.


OTOH I don't like moving code around either, so we can wait until
there's a demonstrated need for the speedup.
msg199872 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-14 11:44
> OTOH I don't like moving code around either, so we can wait until
> there's a demonstrated need for the speedup.

Keep in mind that people who have startup speed problems aren't
likely to open an issue on the tracker about it, let alone
diagnose it enough to put the blame on decimal.
msg216562 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-04-16 19:39
I would like to go ahead with this.  As Antoine mentioned, most
people don't diagnose import problems, especially when they compare
Python 2 against Python 3.

As it turns out, the slowdown is even significant in a simple tcpserver
application that starts a Python script.

Another benefit is that it will be possible to import the Python version
easily, e.g. to check error messages (currently _decimal justs displays
the signal in tracebacks).  I've often wanted that myself instead of
going through the import_fresh_module() ordeal.
msg226700 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-09-10 16:01
New changeset 8bf51cf94405 by Stefan Krah in branch 'default':
Issue #19232: Speed up decimal import.  Additionally, since _decimal is
http://hg.python.org/cpython/rev/8bf51cf94405
msg226701 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-10 17:16
We could speed up the import further by not importing collections
in _decimal.  That could be done once structseq fully implements
the namedtuple protocol (for DecimalTuple).
msg226812 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-09-12 07:43
"We could speed up the import further by not importing collections
in _decimal.  That could be done once structseq fully implements
the namedtuple protocol (for DecimalTuple)."

I suggest to close this issue. I guess that importing decimal is already fast enough, and enhance structseq is a completly different issue. (Is there an open issue, just to get the link?)
msg226816 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-09-12 09:55
I'm fine with closing this. The structseq issue is #1820.
msg229141 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-10-12 11:31
New changeset 75b5617b8dfc by Stefan Krah in branch 'default':
Issue #19232: Fix sys.modules lookup (--without-threads)
https://hg.python.org/cpython/rev/75b5617b8dfc
History
Date User Action Args
2014-10-12 11:31:14python-devsetmessages: + msg229141
2014-09-12 09:55:05skrahsetstatus: open -> closed
versions: + Python 3.5, - Python 3.4
messages: + msg226816

resolution: fixed
stage: patch review -> resolved
2014-09-12 07:43:19vstinnersetmessages: + msg226812
2014-09-10 17:16:23skrahsetmessages: + msg226701
2014-09-10 16:01:20python-devsetnosy: + python-dev
messages: + msg226700
2014-09-09 18:01:35skrahunlinkissue22284 dependencies
2014-08-27 10:17:04skrahlinkissue22284 dependencies
2014-04-16 19:39:57skrahsetmessages: + msg216562
2014-04-16 16:48:14skrahlinkissue21227 dependencies
2013-10-14 11:44:41pitrousetmessages: + msg199872
2013-10-14 11:00:01skrahsetmessages: + msg199864
2013-10-13 16:31:42Arfreversetnosy: + Arfrever
2013-10-13 15:35:56rhettingersetmessages: + msg199724
2013-10-13 10:59:01skrahsetmessages: + msg199691
2013-10-13 08:59:23mark.dickinsonsetmessages: + msg199677
2013-10-12 20:34:38pitrousetnosy: + pitrou
messages: + msg199619
2013-10-12 18:47:36skrahsetfiles: + issue19232-2.patch

messages: + msg199612
2013-10-12 15:32:38barrysetnosy: + barry
2013-10-12 13:32:46skrahsetmessages: + msg199571
2013-10-12 13:24:20vstinnersetmessages: + msg199570
2013-10-12 13:20:30skrahsetmessages: + msg199567
2013-10-12 13:08:07vstinnersetmessages: + msg199564
2013-10-12 13:02:22skrahsetstage: needs patch -> patch review
2013-10-12 13:00:25skrahsetnosy: + rhettinger, mark.dickinson, fijall
messages: + msg199562
2013-10-12 12:49:02skrahsetfiles: + issue19232.patch
keywords: + patch
2013-10-12 12:43:41eric.smithsetnosy: + eric.smith
messages: + msg199558
2013-10-12 12:21:39vstinnersetmessages: + msg199555
2013-10-12 12:21:12vstinnersetnosy: + vstinner
2013-10-12 12:03:19skrahcreate