Cache Digest constants #13730
Cache Digest constants #13730
Conversation
Previously we cached the cipher constants in EVP_CIPHER_fetch(). However, this means we do the caching every time we call that function, even if the core has previusly fetched the cipher and cached it already. This means we can end up re-caching the constants even though they are already present. This also means we could be updating these constants from multiple threads at the same time.
EVP_CIPHER already caches certain constants so that we don't have to query the provider every time. We do the same thing with EVP_MD constants. Without this we can get performance issues, e.g. running "speed" with small blocks of data to digest can spend a long time in EVP_MD_size(), which should be quick. Partialy fixes #13578
|
I've run some tests to see the effect on performance of this change. I ran both the "before" and "after" tests with #13721 applied. I'm seeing quite a lot of variability in the results from one run to the next, so my confidence the precise numbers is quite low. To try and reduce variability as much as possible I "warmed up" my machine before both the "before" and "after" tests by repeating the same speed command 5 times and discarding the results. I then ran the command again 5 times and recorded the results. I compiled with just
The "before" output was:
Which gives an average of 40795.25k The "after" output was:
Which gives an average of 47504.19k. This represents a speed-up of approximately 16% (if you believe the numbers). |
| if (md == NULL) { | ||
| ERR_raise(ERR_LIB_EVP, EVP_R_MESSAGE_DIGEST_IS_NULL); | ||
| return -1; | ||
| } |
beldmit
Dec 22, 2020
Member
Why do we don't check for md here but check in EVP_MD_size?
Why do we don't check for md here but check in EVP_MD_size?
beldmit
Dec 22, 2020
Member
The same question is about EVP_MD_flags
The same question is about EVP_MD_flags
mattcaswell
Dec 22, 2020
|
LGTM |
|
Deprecated functions like EVP_MD_meth_set_result_size() set md->md_size, which is now the cached value. Either they should actually change something in the provider, return an error, or be removed. This PR doesn't actually introduce the problem, but now seems to use existing variables that were not initialized before, but that we did read and then don't do anything with. The functions are currently documented to just return 1. |
|
I think it's a separate issue and these functions should not be applicable to provided digests. |
Exactly. Those functions are deprecated for a reason. They are intended for use when you are setting up your own custom EVP_MD which you have created yourself.
I disagree. They are deprecated and therefore their usage is strongly discouraged. Existing uses will not be broken. I don't see a problem here. |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
Previously we cached the cipher constants in EVP_CIPHER_fetch(). However, this means we do the caching every time we call that function, even if the core has previusly fetched the cipher and cached it already. This means we can end up re-caching the constants even though they are already present. This also means we could be updating these constants from multiple threads at the same time. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #13730)
EVP_CIPHER already caches certain constants so that we don't have to query the provider every time. We do the same thing with EVP_MD constants. Without this we can get performance issues, e.g. running "speed" with small blocks of data to digest can spend a long time in EVP_MD_size(), which should be quick. Partialy fixes #13578 Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #13730)
|
Merged. Thanks! |
EVP_CIPHERalready caches certain constants so that we don't have to query the provider every time. We do the same thing withEVP_MDconstants. Without this we can get performance issues, e.g. running "speed" with small blocks of data to digest can spend a long time inEVP_MD_size(), which should be quick.Partialy fixes #13578
I've also refactored the
EVP_CIPHERconstant caching to move it intoevp_cipher_from_dispatch, i.e. to where theEVP_CIPHERactually gets constructed rather than where it is fetched inEVP_CIPHER_fetch. The problem with the latter is that we construct a cipher once, but we may fetch it many times. We shouldn't have to cache the constants every time we fetch.