classification
Title: remove outdated checks in struct
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: mark.dickinson, meador.inge, serhiy.storchaka, xiang.zhang
Priority: normal Keywords:

Created on 2017-05-02 07:35 by xiang.zhang, last changed 2017-05-15 03:54 by xiang.zhang. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1374 merged xiang.zhang, 2017-05-02 07:40
Messages (7)
msg292724 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-02 07:35
Reading struct's code I find some checks seem outdated. They seemly exists because there are two kinds of integer in 2.x. I wrote a patch to remove them but maybe they exist for other reasons I don't see. Sorry if I misunderstand them. :-(
msg292748 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-05-02 11:34
Could you please run microbenchmarks for affected code to be sure that changes don't hit performance (if for example PyLong_FromLongLong is significantly slower than PyLong_FromLong)?
msg292755 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-02 13:40
There seems no significant regression in performance. A microbenchmark against nu_longlong:

./python3 -m perf timeit --duplicate=1000 --compare-to /home/angwer/repos/cpython/python -s 'import struct; p = struct.Struct("@L")' 'p.unpack(b"d\x00\x00\x00\x00\x00\x00\x00")'
python: ..................... 156 ns +- 10 ns
python3: ..................... 157 ns +- 9 ns

Median +- std dev: [python] 156 ns +- 10 ns -> [python3] 157 ns +- 9 ns: 1.01x slower
Not significant!
msg292762 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-05-02 14:18
I see a tiny 3% regression.

$ ./python -m timeit -s 'import struct; u = struct.Struct("100000Q").unpack; b = b"abcd\0\0\0\0"*100000' 'u(b)'
Unpatched:  50 loops, best of 5: 9.34 msec per loop
Patched:    50 loops, best of 5: 9.66 msec per loop

$ ./python -m timeit -s 'import struct; u = struct.Struct("<100000Q").unpack; b = b"abcd\0\0\0\0"*100000' 'u(b)'
Unpatched:  50 loops, best of 5: 9.37 msec per loop
Patched:    50 loops, best of 5: 9.71 msec per loop

$ ./python -m timeit -s 'import struct; u = struct.Struct(">100000Q").unpack; b = b"\0\0\0\0dcba"*100000' 'u(b)'
Unpatched:  20 loops, best of 5: 11.6 msec per loop
Patched:    20 loops, best of 5: 12 msec per loop
msg292773 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-02 14:56
The test on my box is reverse and stable, python3 is unpatched and python is patched. Anyway, it can't be called significant.

 ./python3 -m perf timeit --compare-to /home/angwer/repos/cpython/python -s 'import struct; u = struct.Struct("100000Q").unpack; b=b"abcd\x00\x00\x00\x00"*100000' 'u(b)'
python: ..................... 2.96 ms +- 0.05 ms
python3: ..................... 3.13 ms +- 0.03 ms

Median +- std dev: [python] 2.96 ms +- 0.05 ms -> [python3] 3.13 ms +- 0.03 ms: 1.06x slower

./python3 -m perf timeit --compare-to /home/angwer/repos/cpython/python -s 'import struct; u = struct.Struct("<100000Q").unpack; b=b"abcd\x00\x00\x00\x00"*100000' 'u(b)'
python: ..................... 2.96 ms +- 0.05 ms
python3: ..................... 3.13 ms +- 0.14 ms

Median +- std dev: [python] 2.96 ms +- 0.05 ms -> [python3] 3.13 ms +- 0.14 ms: 1.06x slower

./python3 -m perf timeit --compare-to /home/angwer/repos/cpython/python -s 'import struct; u = struct.Struct(">100000Q").unpack; b=b"\x00\x00\x00\x00dcba"*100000' 'u(b)'
python: ..................... 3.51 ms +- 0.09 ms
python3: ..................... 3.67 ms +- 0.06 ms

Median +- std dev: [python] 3.51 ms +- 0.09 ms -> [python3] 3.67 ms +- 0.06 ms: 1.05x slower
msg292777 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-05-02 15:21
Your patch LGTM. Current code in _struct.c is ugly. I just wonder if it is worth to optimize PyLong_FromLongLong and PyLong_FromUnsignedLongLong if they are slower than PyLong_FromLong and PyLong_FromUnsignedLong.
msg293668 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-05-15 03:53
New changeset 96f502856796f9310fed7161dc540201a4afc1ee by Xiang Zhang in branch 'master':
bpo-30224: remove outdated checks in struct (#1374)
https://github.com/python/cpython/commit/96f502856796f9310fed7161dc540201a4afc1ee
History
Date User Action Args
2017-05-15 03:54:57xiang.zhangsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-05-15 03:53:53xiang.zhangsetmessages: + msg293668
2017-05-02 15:21:16serhiy.storchakasetmessages: + msg292777
2017-05-02 14:56:31xiang.zhangsetmessages: + msg292773
2017-05-02 14:18:06serhiy.storchakasetmessages: + msg292762
2017-05-02 13:40:44xiang.zhangsetmessages: + msg292755
2017-05-02 11:34:39serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg292748
2017-05-02 07:40:55xiang.zhangsetpull_requests: + pull_request1482
2017-05-02 07:35:05xiang.zhangcreate