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

pickle: constant propagation in _Unpickler_Read() #71243

Closed
vstinner opened this issue May 19, 2016 · 9 comments
Closed

pickle: constant propagation in _Unpickler_Read() #71243

vstinner opened this issue May 19, 2016 · 9 comments
Assignees
Labels
performance Performance or resource usage

Comments

@vstinner
Copy link
Member

BPO 27056
Nosy @vstinner, @serhiy-storchaka
Files
  • unpickle_read.patch
  • unpickle_read-2.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 = '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']

    @vstinner
    Copy link
    Member Author

    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.

    @vstinner vstinner added the performance Performance or resource usage label May 19, 2016
    @serhiy-storchaka
    Copy link
    Member

    +1. Similar optimization is used in marshal.

    Added comments on Rietveld.

    @vstinner
    Copy link
    Member Author

    Updated patch 2 to address Serhiy's comments.

    @serhiy-storchaka
    Copy link
    Member

    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
    Copy link
    Mannequin

    python-dev mannequin commented 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
    Copy link
    Member Author

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

    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
    Copy link
    Mannequin

    python-dev mannequin commented 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
    Copy link
    Member Author

    Serhiy Storchaka:

    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.

    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.

    @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
    performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants