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

Jump to bottom

improve performance of libSSL usage on hashing #74288

Closed
gut mannequin opened this issue Apr 19, 2017 · 11 comments
Closed

improve performance of libSSL usage on hashing #74288

gut mannequin opened this issue Apr 19, 2017 · 11 comments
Assignees
@tiran
Labels
3.7 (EOL) end of life topic-SSL type-feature A feature request or enhancement

Comments

@gut
Copy link
Mannequin

gut mannequin commented Apr 19, 2017

BPO 30102
Nosy @vstinner, @tiran, @gut
PRs
  • bpo-30102: Improve libssl performance on POWER8 for, e.g sha256 #1181
  • bpo-30102: Call OPENSSL_add_all_algorithms_noconf #3112
  • [3.6] bpo-30102: Call OPENSSL_add_all_algorithms_noconf (GH-3112) #3342
  • [2.7] bpo-30102: Call OPENSSL_add_all_algorithms_noconf (GH-3112) #3343
    		•

    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/tiran'
closed_at = <Date 2017-09-05.15:12:53.610>
created_at = <Date 2017-04-19.18:16:07.965>
labels = ['expert-SSL', 'type-feature', '3.7']
title = 'improve performance of libSSL usage on hashing'
updated_at = <Date 2017-09-05.15:28:54.766>
user = 'https://github.com/gut'

    bugs.python.org fields:

    activity = <Date 2017-09-05.15:28:54.766>
actor = 'gut'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2017-09-05.15:12:53.610>
closer = 'christian.heimes'
components = ['SSL']
creation = <Date 2017-04-19.18:16:07.965>
creator = 'gut'
dependencies = []
files = []
hgrepos = []
issue_num = 30102
keywords = []
message_count = 11.0
messages = ['291892', '291895', '291897', '291928', '291980', '301096', '301310', '301317', '301318', '301319', '301320']
nosy_count = 3.0
nosy_names = ['vstinner', 'christian.heimes', 'gut']
pr_nums = ['1181', '3112', '3342', '3343']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30102'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7']
    @gut
    Copy link
    Mannequin Author

    gut mannequin commented Apr 19, 2017

    To correctly pick the best algorithm for the current architecture,
    libssl needs to have OPENSSL_config(NULL) called as described on:
    https://wiki.openssl.org/index.php/Libcrypto_API

    This short change lead to a speedup of 50% on POWER8 when using
    hashlib.sha256 digest functionality as it now uses a SIMD approach that was already existing but not used by cpython.

    @gut gut mannequin added the 3.7 (EOL) end of life label Apr 19, 2017
    @gut gut mannequin assigned tiran Apr 19, 2017
    @gut gut mannequin added topic-SSL type-feature A feature request or enhancement labels Apr 19, 2017
    @tiran
    Copy link
    Member

    tiran commented Apr 19, 2017

    This small change also changes behavior of OpenSSL dramatically. What are your Python and OpenSSL versions? How did you profile the performance of the hashing functions?

    If OpenSSL_config() really improves performance on relevant platforms, then we should be consistent and call it in _ssl.c, too.

    @gut
    Copy link
    Mannequin Author

    gut mannequin commented Apr 19, 2017

    Christian Heimes added the comment:

    This small change also changes behavior of OpenSSL dramatically. What
    are your Python and OpenSSL versions?

    I tested by compiling my own python3 (8aaf499) against its parent (d6d344d) For python2, I made the same by compiling the 2.7.13 as a baseline and results were consistent with the python I have on Ubuntu 17.10 (3.5.3 and 2.7.13).

    How did you profile the
    performance of the hashing functions?

    I compared the timings between my patched vs patchless python versions by using a 630MB file to calculate sha256 as:

    perf stat -r 10 Python-2.7.13/python -c "import hashlib; print(hashlib.sha256(open('ubuntu-16.10-server-ppc64el.iso','rb').read()).hexdigest())"

    The speedup of ~50% is given due to patchless taking 3.082156387s and patched taking 2.027484800s (errors are less than 1%)

    I also noticed what changed on a code level by checking with gdb what is being executed on the sha loop. After my patch I see altivec (SIMD) functions being used on SHA main loop.

    If OpenSSL_config() really improves performance on relevant platforms,
    then we should be consistent and call it in _ssl.c, too.

    I'll check that then. Thanks for the hint.

    Ps: please note this behavior is noticed on a POWER8 machine. I'm not an OpenSSL expert so I don't know if there are now more optimizations enabled on other architectures as well.

    @vstinner
    Copy link
    Member

    This small change also changes behavior of OpenSSL dramatically.

    What do you mean by "dramatically"? What does a openssl.cnf configuration contain?

    Do other applications using OpenSSL call OPENSSL_config(NULL)?

    Since OPENSSL_config() accepts a filename, maybe a first step would be to expose the function as ssl.OPENSSL_config(filename) to allow user to load *explicitly* a configuration file? ssl.OPENSSL_config() would call OPENSSL_config(NULL). Would it work for you, Gustavo?

    --

    More links:

    @gut
    Copy link
    Mannequin Author

    gut mannequin commented Apr 20, 2017

    Since OPENSSL_config() accepts a filename, maybe a first step would be
    to expose the function as ssl.OPENSSL_config(filename) to allow user to
    load *explicitly* a configuration file? ssl.OPENSSL_config() would call
    OPENSSL_config(NULL). Would it work for you, Gustavo?

    It would work, I would just wait for such a decision.

    BTW, that interface is deprecated as we're discussing on the issue on GitHub:
    https://github.com/openssl/openssl/blob/cda3ae5bd0798c56fef5a5c1462d51ca1776504e/doc/crypto/OPENSSL_config.pod#notes

    I'll continue analyzing this issue there.

    @gut
    Copy link
    Mannequin Author

    gut mannequin commented Sep 1, 2017

    Is there any news on this issue? The PR 3112 also seems to be frozen at the moment. Is there some kind of code freeze happening at the moment that no reviews are taking place?

    @tiran
    Copy link
    Member

    tiran commented Sep 5, 2017

    New changeset c941e62 by Christian Heimes in branch 'master':
    bpo-30102: Call OPENSSL_add_all_algorithms_noconf (bpo-3112)
    c941e62

    @tiran
    Copy link
    Member

    tiran commented Sep 5, 2017

    New changeset 2ddea0f by Christian Heimes in branch '3.6':
    [3.6] bpo-30102: Call OPENSSL_add_all_algorithms_noconf (GH-3112) (bpo-3342)
    2ddea0f

    @tiran
    Copy link
    Member

    tiran commented Sep 5, 2017

    New changeset 7daa45d by Christian Heimes in branch '2.7':
    [2.7] bpo-30102: Call OPENSSL_add_all_algorithms_noconf (GH-3112) (bpo-3343)
    7daa45d

    @tiran
    Copy link
    Member

    tiran commented Sep 5, 2017

    Thanks for your persistence and your initial patch.

    @tiran tiran closed this as completed Sep 5, 2017
    @gut
    Copy link
    Mannequin Author

    gut mannequin commented Sep 5, 2017

    No worries. I thank you also for reviewing all these changesets. I'm glad it worked in the end.

    @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
    Assignees

    @tiran tiran

    Labels
    3.7 (EOL) end of life topic-SSL type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @tiran