bpo-35941: Fix performance regression in new code #12610
Conversation
tiran
force-pushed
the
bpo-35941-performance
branch
from
15e5668 to
acf0ea5
Compare
tiran
force-pushed
the
bpo-35941-performance
branch
from
acf0ea5 to
e78c191
Compare
tiran
force-pushed
the
bpo-35941-performance
branch
from
e78c191 to
7c5f0e4
Compare
|
Looks like one of the elements is indeed not hashable, which is why you need the list. But a set should be able to compare equal properly even within a tuple, so the previous code was fine (if slow). If you want to go as far as using a dict keyed off the certificate bytes and storing all matches in lists then you could reduce the number of comparisons significantly, but I don't think this is necessarily performance sensitive enough to be worth it. Maybe change the Python code to cache the result unless it's explicitly called again? |
|
The Windows cert store can contain more than a hundred unique root certs. We'll end up with 5,000 to 10,000 comparisons to build a list of unique items. It's a performance issue. I have figured out why it's failing. |
tiran
force-pushed
the
bpo-35941-performance
branch
from
7c5f0e4 to
9774065
Compare
|
Looks like it's fine apart from a type check in |
Modules/_ssl.c
Outdated
There was a problem hiding this comment.
You could remove this else statement (also in the corresponding code in _ssl_enum_crls_impl).
Accumulate certificates in a set instead of doing a costly list contain operation. A Windows cert store can easily contain over hundred certificates. The old code would result in way over 5,000 comparison operations Signed-off-by: Christian Heimes <[email protected]>
tiran
force-pushed
the
bpo-35941-performance
branch
from
9774065 to
a6e73a6
Compare
|
Sorry, @tiran and @zooba, I could not cleanly backport this to |
|
Sorry @tiran and @zooba, I had trouble checking out the |
…GH-12610) Accumulate certificates in a set instead of doing a costly list contain operation. A Windows cert store can easily contain over hundred certificates. The old code would result in way over 5,000 comparison operations Signed-off-by: Christian Heimes <[email protected]>
|
GH-15803 is a backport of this pull request to the 3.8 branch. |
…GH-12610) Accumulate certificates in a set instead of doing a costly list contain operation. A Windows cert store can easily contain over hundred certificates. The old code would result in way over 5,000 comparison operations Signed-off-by: Christian Heimes <[email protected]>
|
GH-15804 is a backport of this pull request to the 3.7 branch. |
Accumulate certificates in a set instead of doing a costly list contain operation. A Windows cert store can easily contain over hundred certificates. The old code would result in way over 5,000 comparison operations Signed-off-by: Christian Heimes <[email protected]>
Accumulate certificates in a set instead of doing a costly list contain operation. A Windows cert store can easily contain over hundred certificates. The old code would result in way over 5,000 comparison operations Signed-off-by: Christian Heimes <[email protected]>
Accumulate certificates in a set instead of doing a costly list contain operation. A Windows cert store can easily contain over hundred certificates. The old code would result in way over 5,000 comparison operations Signed-off-by: Christian Heimes <[email protected]>
7 participants
Accumulate certificates in a set instead of doing a costly list contain
operation. A Windows cert store can easily contain over hundred
certificates. The old code would result in way over 5,000 comparison
operations.
Signed-off-by: Christian Heimes [email protected]
Untested, uncompiled C code. Let's see what the Windows build bot has to say.
https://bugs.python.org/issue35941