Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dict: Optimize PyDict_GetItemString() #73481

Closed
methane opened this issue Jan 17, 2017 · 5 comments
Closed

dict: Optimize PyDict_GetItemString() #73481

methane opened this issue Jan 17, 2017 · 5 comments
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@methane
Copy link
Member

methane commented Jan 17, 2017

BPO 29295
Nosy @rhettinger, @vstinner, @methane, @serhiy-storchaka
Files
  • dict_getitemascii.patch
  • 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']

    @methane
    Copy link
    Member Author

    methane commented 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):

    • telco: 24.2 ms +- 0.4 ms -> 21.8 ms +- 0.7 ms: 1.11x faster (-10%)
    • xml_etree_parse: 315 ms +- 17 ms -> 302 ms +- 14 ms: 1.04x faster (-4%)
    • logging_simple: 31.6 us +- 0.3 us -> 30.4 us +- 0.3 us: 1.04x faster (-4%)
    • mako: 41.6 ms +- 0.7 ms -> 40.3 ms +- 0.4 ms: 1.03x faster (-3%)
    • logging_format: 36.5 us +- 0.3 us -> 35.5 us +- 0.4 us: 1.03x faster (-3%)
    • float: 297 ms +- 4 ms -> 289 ms +- 4 ms: 1.03x faster (-3%)
    • scimark_monte_carlo: 276 ms +- 10 ms -> 269 ms +- 7 ms: 1.02x faster (-2%)
    • regex_effbot: 5.31 ms +- 0.37 ms -> 5.19 ms +- 0.06 ms: 1.02x faster (-2%)
    • pickle_pure_python: 1.32 ms +- 0.02 ms -> 1.29 ms +- 0.02 ms: 1.02x faster (-2%)
    • scimark_sor: 525 ms +- 9 ms -> 514 ms +- 8 ms: 1.02x faster (-2%)
    • richards: 180 ms +- 3 ms -> 176 ms +- 2 ms: 1.02x faster (-2%)

    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.

    @methane methane added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Jan 17, 2017
    @serhiy-storchaka
    Copy link
    Member

    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
    Copy link
    Member Author

    methane commented 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
    Copy link
    Member Author

    methane commented Jan 17, 2017

    Close this issue for now, until profiler shows me PyDict_XxxxxString.

    @methane methane closed this as completed Jan 17, 2017
    @vstinner
    Copy link
    Member

    Close this issue for now, until profiler shows me PyDict_XxxxxString.

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants