Repository navigation
pickle: constant propagation in _Unpickler_Read() #71243
Description
| BPO | 27056 |
|---|---|
| Nosy | @vstinner, @serhiy-storchaka |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/vstinner'
closed_at = <Date 2016-05-20.09:45:57.134>
created_at = <Date 2016-05-19.12:02:29.287>
labels = ['performance']
title = 'pickle: constant propagation in _Unpickler_Read()'
updated_at = <Date 2016-05-20.19:19:20.651>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2016-05-20.19:19:20.651>
actor = 'vstinner'
assignee = 'vstinner'
closed = True
closed_date = <Date 2016-05-20.09:45:57.134>
closer = 'vstinner'
components = []
creation = <Date 2016-05-19.12:02:29.287>
creator = 'vstinner'
dependencies = []
files = ['42896', '42914']
hgrepos = []
issue_num = 27056
keywords = ['patch']
message_count = 9.0
messages = ['265852', '265858', '265920', '265922', '265925', '265926', '265946', '265952', '265953']
nosy_count = 3.0
nosy_names = ['vstinner', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue27056'
versions = ['Python 3.6']
Activity
vstinner commentedon May 19, 2016
According to Linux perf, the unpickle_list benchmark (of the CPython benchmark suite) heavily depends on the performance of load() and _Unpickler_Read() functions. While running benchmarks with PGO+LTO compilation, I noticed a difference around 5% because one build uses constant propation on _Unpickler_Read() (fast), wheras another build doesn't (slow).
Depending on the result of the PGO, depending on the OS and the compiler, you may or may not get this nice optimization. I propose to implement it manually to not depend on the compiler.
Attached short patch implements manually the optimization. It looks to have a big impact on unpickle_list, but no impact (benchmark is not significant) on fastunpickle and pickle_list:
---
$ taskset -c 3 python3 perf.py ../ref_default/rel/python ../default/rel/python -b unpickle_list,fastunpickle,pickle_list --rigorous -v (...) Report on Linux smithers 4.4.9-300.fc23.x86_64 #1 SMP Wed May 4 23:56:27 UTC 2016 x86_64 x86_64 Total CPU cores: 4### fastunpickle ###
Avg: 0.527359 +/- 0.005932 -> 0.518548 +/- 0.00953: 1.02x faster
Not significant
### pickle_list ###
Avg: 0.269307 +/- 0.017465 -> 0.266015 +/- 0.00423: 1.01x faster
Not significant
### unpickle_list ###
Avg: 0.255805 +/- 0.006942 -> 0.231444 +/- 0.00394: 1.11x faster
Significant (t=21.58)
---
It would be interesting to also evaluate the computed goto optimization for the load() function. (And also try computed goto for the re/_sre module, but that's a different issue.)
I tuned my system and patched perf.py (of the CPython benchmark suite) to get stable benchmark results.
serhiy-storchaka commentedon May 19, 2016
+1. Similar optimization is used in marshal.
Added comments on Rietveld.
vstinner commentedon May 20, 2016
Updated patch 2 to address Serhiy's comments.
serhiy-storchaka commentedon May 20, 2016
I forgot to note that the comment before _Unpickler_Read() becomes not correct, but you already addressed this in the second patch. unpickle_read-2.patch LGTM.
Few months ago I tried to optimize reading in pickle, but didn't see significant benefit. After committing your patch I'm going to revive my patch.
python-dev commentedon May 20, 2016
New changeset f9b85b47f9c8 by Victor Stinner in branch 'default':
Optimize pickle.load() and pickle.loads()
https://hg.python.org/cpython/rev/f9b85b47f9c8
vstinner commentedon May 20, 2016
Serhiy Storchaka: "I forgot to note that the comment before _Unpickler_Read() becomes not correct, but you already addressed this in the second patch. unpickle_read-2.patch LGTM."
Cool, pushed.
"Few months ago I tried to optimize reading in pickle, but didn't see significant benefit. After committing your patch I'm going to revive my patch."
Cool too, keep me in touch.
serhiy-storchaka commentedon May 20, 2016
I think that integer overflow in _Unpickler_Read() is possible. n is read from file and can be arbitrary (up to PY_SSIZE_T_MAX). This likely cause raising an exception later, but integer overflow itself causes undefined behavior, and we should avoid it.
python-dev commentedon May 20, 2016
New changeset 3d7b7aa89437 by Victor Stinner in branch 'default':
Issue bpo-27056: Fix _Unpickler_Read() to avoid integer overflow
https://hg.python.org/cpython/rev/3d7b7aa89437
vstinner commentedon May 20, 2016
Serhiy Storchaka:
Hum, I understood that it's ok since numbers should be signed, but in
fact I'm not confident that n is always signed. You are right, it's
better to use your code to avoid the integer overflow. I pushed a fix.