Issue 25147: Enum: remove dependency on OrderedDict (original) (raw)

process

Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ethan.furman Nosy List: barry, eli.bendersky, eric.snow, ethan.furman, python-dev, r.david.murray, rhettinger, scoder, serhiy.storchaka, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2015-09-17 07:18 by ethan.furman, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
issue25147.stoneleaf.01.patch ethan.furman,2015-09-17 07:24 review
delayed_ordered_dict.patch vstinner,2015-09-17 10:34 review
Messages (18)
msg250875 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-09-17 07:18
Pulling in collections.OrderedDict has a significant startup cost (from what I've heard, and can easily believe by glancing at all the imports in collections.__init__). By keeping a separate list for the Enum member names using Enum in the stdlib becomes more attractive.
msg250878 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-17 09:13
This change has visible side effect: __members__ is no longer ordered. This should be reflected in the documentation and in What's New.
msg250879 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-17 10:34
> This change has visible side effect: __members__ is no longer ordered. This property of part of the PEP 435 (enum): "The special attribute __members__ is an ordered dictionary mapping names to members." IMHO if we really want to change this, it must be discussed on the python-dev mailing list. To keep __members__ ordered, we can delay the creation of the OrderedDict at the first access to __members__: start with a dict, and use the ordered _all_member_names_ to created the ordered dictionary. Attached patch implements this idea. I checked enum.Enum and enum.IntEnum classes and the @enum.unique decorated: __members__ is not used to declare the class. Hum, I had to modify a little bit @enum.unique to not access __members__.
msg250895 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2015-09-17 16:14
OrderedDict has a C implementation now. So try the following: try: from _collections import OrderedDict except ImportError: from collections import OrderedDict
msg250898 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-09-17 16:40
Is pulling in `_collections` not as resource intensive as pulling in `collections`?
msg250900 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-09-17 17:22
_collections exists because it contains only the stuff that is needed at python startup. So it is loaded regardless, and thus is very cheap to import elsewhere :)
msg250904 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-17 17:43
_collections exists because it contains C implementations of some collections classes or helpers. _collections itself is imported only in collections and threading. And in threading the same idiom as proposed by Eric is used: try: from _collections import deque as _deque except ImportError: from collections import deque as _deque What is the cost of importing OrderedDict at all? Ethan, can you provide any measurement results?
msg250905 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-09-17 17:55
_collections sounds cool, but the flip side is any python without the C implemntation would still have the slower startup, right? No, I don't have measurements -- just that I have heard importing collections can have an effect on startup time. Of course, it's only a cost you pay once, so maybe it's not a big deal. On the other hand, I could make __members__ a caching property that doesn't import OrderedDict until it is used, and not use it internally -- and I can't think of any reason why the stdlib would need to access __members__ itself.
msg250909 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-17 18:27
I tried to measure import time with different patches and didn't notice any difference.
msg250911 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-09-17 18:39
In that case I'll go with _collections; if performance does become an issue with other pythons later we can add the caching property. Thanks for all the insights.
msg250917 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-17 20:07
If there is no any evidences, we shouldn't change a code.
msg250933 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-09-18 04:53
Serhiy, your objection is noted, thank you.
msg250934 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-09-18 05:04
New changeset b77916d2d7cc by Ethan Furman in branch 'default': Close : use C implementation of OrderedDict https://hg.python.org/cpython/rev/b77916d2d7cc
msg250935 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-09-18 05:14
That change could use a comment stating why it's doing things that way instead of the 'obvious' way.
msg250938 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-09-18 05:55
> _collections sounds cool, but the flip side is any python without the C implemntation would still have the slower startup, right? I wouldn't bother too much with that, certainly not given the order we are talking about here. Jython's startup time is slowed down mostly by the JVM startup time anyway. And if you use PyPy then it's exactly because you do *not* care about the startup time but about performance improvements for long running code.
msg250939 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-09-18 05:56
New changeset c0363f849624 by Ethan Furman in branch 'default': Issue 25147: add reason for using _collections https://hg.python.org/cpython/rev/c0363f849624
msg250942 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-18 06:25
But the comment is false. That change doesn't reduce startup cost.
msg250943 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-09-18 06:33
Let's say the change minimises the dependencies. That is a reasonable goal, too.
History
Date User Action Args
2022-04-11 14:58:21 admin set github: 69334
2015-09-18 06:33:24 scoder set messages: +
2015-09-18 06:25:14 serhiy.storchaka set messages: +
2015-09-18 05:56:13 python-dev set messages: +
2015-09-18 05:55:47 scoder set nosy: + scodermessages: +
2015-09-18 05:14:35 zach.ware set nosy: + zach.waremessages: +
2015-09-18 05:04:21 python-dev set status: open -> closednosy: + python-devmessages: + resolution: fixedstage: patch review -> resolved
2015-09-18 04:53:42 ethan.furman set messages: +
2015-09-17 20:07:00 serhiy.storchaka set messages: +
2015-09-17 18:39:51 ethan.furman set messages: +
2015-09-17 18:27:22 serhiy.storchaka set messages: +
2015-09-17 17:55:40 ethan.furman set messages: +
2015-09-17 17:44:00 serhiy.storchaka set nosy: + rhettingermessages: +
2015-09-17 17:22:41 r.david.murray set nosy: + r.david.murraymessages: +
2015-09-17 16:40:39 ethan.furman set messages: +
2015-09-17 16:14:04 eric.snow set nosy: + eric.snowmessages: +
2015-09-17 10:34:35 vstinner set files: + delayed_ordered_dict.patchnosy: + vstinnermessages: +
2015-09-17 09:13:39 serhiy.storchaka set nosy: + serhiy.storchakamessages: +
2015-09-17 07:24:21 ethan.furman set files: + issue25147.stoneleaf.01.patchkeywords: + patch
2015-09-17 07🔞27 ethan.furman create