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

adding a fetch test to perf tools #193

Closed
wants to merge 5 commits into from
Closed

Conversation

nhorman
Copy link
Contributor

@nhorman nhorman commented May 7, 2024

Add a perf test to record how many EVP fetches we can do in unit time

Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

One subtle numeric issue.

perf/evp_fetch.c Outdated Show resolved Hide resolved
perf/evp_fetch.c Outdated Show resolved Hide resolved
perf/evp_fetch.c Show resolved Hide resolved
perf/evp_fetch.c Show resolved Hide resolved
perf/evp_fetch.c Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented May 10, 2024

Needs rebase now

@nhorman
Copy link
Contributor Author

nhorman commented May 10, 2024

done

perf/evp_fetch.c Outdated Show resolved Hide resolved
return EXIT_FAILURE;
}

ctx = OSSL_LIB_CTX_new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should have a "warm up" option that would do one fetch here outside the measurements. The initial fetch on a libctx with no providers loaded will load the default provider implicitly and fighting for that could skew the numbers fairly visibly. I.e. it would be interesting to see the differences in the numbers. Or instead we could explicitly load the default provider with OSSL_PROVIDER_load().

Copy link
Contributor Author

@nhorman nhorman May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that would depend on the case we want to measure. I could envision wanting to measure both the fast path (where a fetch would always hit in the cache), and the slow path (where a fetch would never hit in the cache)

Can we save that for a subsequent PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another PR I think.

IMO a warm up isn't going to impact the measurement significantly given a large number of iterations per thread.

@nhorman nhorman requested a review from t8m May 17, 2024 15:16
Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found just two nits. otherwise looks good. it's up to you if you address those nits or not. I don't insist.

}

if (fetch_type != NULL) {
exclusive_fetch_alg = strstr(fetch_type, ":");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strchr()? I think strstr() is a big hammer here.

perf/evp_fetch.c Outdated Show resolved Hide resolved
perf/evp_fetch.c Outdated
#include <openssl/core_names.h>
#include "perflib/perflib.h"

#define NUM_CALLS_PER_TEST 100000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider raising this number. Even 10000000 calls takes just a few seconds on my laptop.

@t8m
Copy link
Member

t8m commented May 20, 2024

Merged. Thank you for your contribution.

@t8m t8m closed this May 20, 2024
paulidale pushed a commit to paulidale/tools that referenced this pull request May 20, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#193)
paulidale pushed a commit to paulidale/tools that referenced this pull request May 20, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#193)
paulidale pushed a commit to paulidale/tools that referenced this pull request May 20, 2024
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#193)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants