Repository navigation
dict: Optimize PyDict_GetItemString() #73481
Description
| BPO | 29295 |
|---|---|
| Nosy | @rhettinger, @vstinner, @methane, @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 = None
closed_at = <Date 2017-01-17.12:11:06.570>
created_at = <Date 2017-01-17.10:47:42.801>
labels = ['interpreter-core', '3.7', 'performance']
title = 'dict: Optimize PyDict_GetItemString()'
updated_at = <Date 2017-01-17.13:27:12.536>
user = 'https://github.com/methane'bugs.python.org fields:
activity = <Date 2017-01-17.13:27:12.536>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-01-17.12:11:06.570>
closer = 'methane'
components = ['Interpreter Core']
creation = <Date 2017-01-17.10:47:42.801>
creator = 'methane'
dependencies = []
files = ['46313']
hgrepos = []
issue_num = 29295
keywords = ['patch']
message_count = 5.0
messages = ['285631', '285633', '285635', '285636', '285639']
nosy_count = 4.0
nosy_names = ['rhettinger', 'vstinner', 'methane', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = None
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue29295'
versions = ['Python 3.7']
Activity
methane commentedon Jan 17, 2017
PyDict_GetItemString() is heavily used, especially from keyword argument parsing.
Current implementation creates temporary string for key object.
This patch avoid the temporary key string when passed C string is ASCII.
This benchmark is based on a8563ef0eb8a, so PyDict_GetItemString() calls for
parsing positional arguments is reduced already.
$ ../python -m perf compare_to -G --min-speed 2 default.json patched.json Slower (1): - scimark_lu: 430 ms +- 21 ms -> 446 ms +- 23 ms: 1.04x slower (+4%)Faster (11):
Benchmark hidden because not significant (52): ...
Performance difference of telco is bit surprising.
Profiler shows the difference is from
print(t, file=outfil)(here: https://github.com/python/performance/blob/master/performance/benchmarks/bm_telco.py#L79 )Until most common builtin functions are converted to FASTCALL, this patch has significant
performance gain.
serhiy-storchaka commentedon Jan 17, 2017
It looks to me that PyDict_GetItemString(), PyObject_GetAttrString(), etc are mainly for backward compatibility and for using in performance non-critical code. Performance critical code caches string objects.
The only code that heavily used PyDict_GetItemString() was parsing keyword arguments in PyArg_ParseTupleAndKeywords(). But this API was replaced with more efficient _PyArg_ParseTupleAndKeywordsFast() and _PyArg_ParseStackAndKeywords() for internal use. I think something similar will be exposed as public API when it become enough mature. bpo-29029 made PyArg_ParseTupleAndKeywords() much less using PyDict_GetItemString().
PyDict_GetItemString() can be used with non-ASCII C strings. They are decoded with UTF-8. The patch works incorrectly in this case.
I afraid that adding more and more specialized code in Objects/dictobject.c can slow down other functions in this file. And this makes the maintenance harder.
methane commentedon Jan 17, 2017
This patch checks passed C string is ascii or not.
But I don't want make dict complex too. telco is more faster with bpo-29296.
Most common builtin functions are not METH_KEYWORDS when it merged.
methane commentedon Jan 17, 2017
Close this issue for now, until profiler shows me PyDict_XxxxxString.
vstinner commentedon Jan 17, 2017
I like Serhiy's rationale. We should try to avoid PyDict_GetItemString() wheneve possible. If PyDict_GetItemString() becomes a clear bottleneck, we can discuss again optimizing it. But in the meanwhile, I prefer to use anothe rule: an optimization should not modify the behaviour of a function. That's why I didn't optimize OrderedDict.pop() yet (it changed the docstring, AC should be enhanced for this case).
So yeah, let's close this one.