document that sprite groups are ordered in python 3.6 and above #2336
Conversation
|
Apart from writing some micro benchmarks, one way to do sprite benchmarks is with multiple runs of It's probably good to also test with something else, like with a game that uses sprites. |
|
This implementation seems very sensible to me. I believe regular python dicts will be slightly faster (from looking over other benchmarking data) but it's likely that if you care that much about speed and are using sprites you will be using python 3.7+ and if you care about speed to that degree and are using python 2.7 you probably aren't using pygame's sprite module anyway. Basically I feel like the consistent behaviour across versions will be more widely useful than slightly slowing down performance of older python 3 versions and 2.7 users using sprites. I expect one day a C version of a sprite group could do some of the looping draw to blit parts a bit faster but that would likely be part of a wider sprite module refactor (that may become dominated by sorting out texture sharing and that sort of thing anyway). |
|
Let's just benchmark it, and then we can see if the trade off is worth it. |
|
I started the benchmarking, will try to finish it today. |
|
If you read this hacker news thread: https://news.ycombinator.com/item?id=22266173 it seems that the implementation was changed for dictionaries to be insertion ordered for CPython in 3.6. But the change wasn't made to the actual python language spec until 3.7 So you could still have valid 3.6 python code with unordered dictionaries if it was running on a non-CPython interpreter. |
|
Cool, thanks. If you add a unit test for the sorting, then the CI can run on different versions of python to confirm the ordering. There's some other dict uses in sprite.py that are likely affected by the same issue. Some microbenchmark timings for dict assignment in python 3.8 on mac. In [1]: %timeit dict()
126 ns ± 1.16 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [2]: %timeit {}
23.5 ns ± 0.123 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [3]: %timeit {}
23.5 ns ± 0.123 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [4]: from collections import OrderedDict
In [5]: %timeit OrderedDict()
121 ns ± 0.351 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [6]: %timeit OrderedDict()
120 ns ± 0.597 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [7]: %timeit dict()
125 ns ± 0.654 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [8]: %timeit dict()
126 ns ± 1.23 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [9]: %timeit OrderedDict()
121 ns ± 0.64 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [12]: import timeit
In [13]: s= """
...: if USE_ORDEREDDICT:
...: a = OrderedDict()
...: else:
...: a = {}
...: """
In [21]: timeit.timeit(s, setup="from collections import OrderedDict", globals={"USE_ORDEREDDICT":1})
Out[21]: 0.1377905770000325
In [22]: timeit.timeit(s, setup="from collections import OrderedDict", globals={"USE_ORDEREDDICT":0})
Out[22]: 0.04726486300000943
In [23]: timeit.timeit(s, setup="from collections import OrderedDict", globals={"USE_ORDEREDDICT":0})
Out[23]: 0.047848736999981156
In [24]: timeit.timeit("a={}", setup="from collections import OrderedDict", globals={"USE_ORDEREDDICT":0})
Out[24]: 0.02688187400002562
In [25]: timeit.timeit("a={}", setup="from collections import OrderedDict", globals={"USE_ORDEREDDICT":0})
Out[25]: 0.02763441999996985 |
Python 2.7.17:
Python 3.4.4:
|
|
Looks like the using the basic python dict runs about 10% - 30% faster on these tests. Not super great for OrderedDict. Perhaps if we do go ahead with this we should also add an environment variable that allows users to override USE_ORDEREDDICT. Something like |
|
Thanks for testing that @zoldalma999. I'd like to repeat the tests on some slower gfx hardware, and another sprite using game just to be sure. I'll get around to it in a few days. If it's a similar result, I'd suggest we don't make the change. Instead, people who care could use a mixin class or a monkey patch. |
|
If we dont add orderedDicts i would still edit the docs so it tells you which python version you can use to have an ordered group since currently this is whats in the docs:
Perhaps something like this
would be nicer. |
|
I think we should not include this change, just because of the noticeable performance penalty. The doc change is a good idea. I'll also note in the issue that a subclass of the group that uses an ordered dict can be used for people who want to rely on ordering in cpython 2.7.
Yeah, that text would be nicer. Note however, I think we should use 3.6. Since although it wasn't guaranteed as part of a specification, 3.6 was ordered. If you agree, we can close this PR and I'll make the notes in the issue about the changes. If you instead think the benefit of ordering outweighs the performance penalty I'm open to including the PR. Can I leave the decision to you @zoldalma999 ? |
|
I think either we should ditch it or make it optional and default to off. What MyreMylar suggested seems like a good way if we want to chose the latter. |
|
Ok. Well, ditching it seems the easier option for now :) |
|
Thanks @zoldalma999 |
Related to #1951
I will do benchmarks if the tests pass.