This change adds run-perf.sh script. The script runs all tools #207
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Here is the sample output. I used a modified version of evp_fetch
randbytes
handshake
sslnew
newrawkey
rsasign
x509storeissuer
providerdoall
rwlocks-rlock
rwlocks-wlock
pkeyread-dh-der
pkeyread-dhx-der
pkeyread-dsa-der
pkeyread-ec-der
pkeyread-rsa-der
pkeyread-x25519-der
pkeyread-dh-pem
pkeyread-dhx-pem
pkeyread-dsa-pem
pkeyread-ec-pem
pkeyread-rsa-pem
pkeyread-x25519-pem
|
mattcaswell
requested changes
Jun 20, 2024
| ----------- | ||
|
|
||
| Script which collects performance stats by running all tools above. It tests | ||
| OpenSSL versions 1.1.1 3.0 and master. |
There was a problem hiding this comment.
What about 3.1, 3.2, 3.3? Aren't we interested in those too? In particular we should be interested in master vs the latest released version (3.3) - so perhaps 1.1.1, 3.0, 3.3 and master are sufficient.
There was a problem hiding this comment.
OK, so I will add 3.3 to the script.
perf/run-perf.sh
Outdated
| } | ||
|
|
||
| mkdir -p $RESULTS | ||
| rm -rf $RESULTS/* |
There was a problem hiding this comment.
Yikes....better hope no one ever clears the RESULTS setting above or this could end badly. Should we sanity check $RESULTS?
There was a problem hiding this comment.
I think TMPDIR is the answer here.
perf/run-perf.sh
Outdated
| make | ||
| fi | ||
| popd | ||
| for THREADS in 1 2 4 8 16 32 64 ; do |
There was a problem hiding this comment.
I wonder if 64 is really a high enough thread count. Previously we've tested up to 500 or 1000
There was a problem hiding this comment.
I'm using what @nhorman put into his fist version of the script. The whole script is based on his work in fact.
IMO the thread count should be aligned with CPU-cores particular system has. This should be customized once script is deployed to test machines.
There was a problem hiding this comment.
IMO the thread count should be aligned with CPU-cores particular system has. This should be customized once script is deployed to test machines.
It is interesting to see values for numbers of threads around the number of physical cores and number of hyperthreads. However it is also interesting to see the value for some much higher number of threads. Not sure we need to measure both 500 and 1000 as these numbers should not differ that much.
There was a problem hiding this comment.
I think running 1 or 2 powers of two above the highest number of cpus we reasonably expect a system to have makes the most sense. I think the perf system we have waiting for us has 96 cpus, so limiting to 128 or 256 is sensible
t8m
reviewed
Jun 20, 2024
perf/run-perf.sh
Outdated
| echo "### $TOOL" >> $OUT | ||
| echo '' >> $OUT | ||
| # print table header | ||
| echo -n '|thread count| numder of iterations |' >> $OUT |
t8m
requested changes
Jun 20, 2024
perf/run-perf.sh
Outdated
Comment on lines
115
to
120
| for TOOL in evp_fetch randbytes handshake sslnew newrawkey rsasign \ | ||
| x509storeissuer providerdoall rwlocks-rlock rwlocks-wlock \ | ||
| pkeyread-dh-der pkeyread-dhx-der pkeyread-dsa-der pkeyread-ec-der \ | ||
| pkeyread-rsa-der pkeyread-x25519-der pkeyread-dh-pem pkeyread-dhx-pem \ | ||
| pkeyread-dsa-pem pkeyread-ec-pem pkeyread-rsa-pem \ | ||
| pkeyread-x25519-pem ; do |
There was a problem hiding this comment.
Put the list of tools into a variable so you do not have to duplicate it below
perf/run-perf.sh
Outdated
Comment on lines
194
to
199
| for TOOL in evp_fetch randbytes handshake sslnew newrawkey rsasign \ | ||
| x509storeissuer providerdoall rwlocks-rlock rwlocks-wlock \ | ||
| pkeyread-dh-der pkeyread-dhx-der pkeyread-dsa-der pkeyread-ec-der \ | ||
| pkeyread-rsa-der pkeyread-x25519-der pkeyread-dh-pem pkeyread-dhx-pem \ | ||
| pkeyread-dsa-pem pkeyread-ec-pem pkeyread-rsa-pem \ | ||
| pkeyread-x25519-pem ; do |
There was a problem hiding this comment.
Please avoid the duplication of the tools
nhorman
requested changes
Jun 20, 2024
perf/run-perf.sh
Outdated
| # https://www.openssl.org/source/license.html | ||
| # | ||
|
|
||
| set -x |
There was a problem hiding this comment.
I usually use this for debugging, I'm not sure we need it on all the time
perf/run-perf.sh
Outdated
| make | ||
| fi | ||
| popd | ||
| for THREADS in 1 2 4 8 16 32 64 ; do |
There was a problem hiding this comment.
I think running 1 or 2 powers of two above the highest number of cpus we reasonably expect a system to have makes the most sense. I think the perf system we have waiting for us has 96 cpus, so limiting to 128 or 256 is sensible
perf/run-perf.sh
Outdated
| for TOOL in evp_fetch randbytes handshake sslnew newrawkey rsasign \ | ||
| x509storeissuer providerdoall rwlocks-rlock rwlocks-wlock \ | ||
| pkeyread-dh-der pkeyread-dhx-der pkeyread-dsa-der pkeyread-ec-der \ | ||
| pkeyread-rsa-der pkeyread-x25519-der pkeyread-dh-pem pkeyread-dhx-pem \ |
There was a problem hiding this comment.
is the intent here to add scripts or something to represent utilties like pkeyread-dh-der? I ask because I don't see these utilities added anywhere. It would actually be reasonable to do as that would let us add scripts that set various options for the tool in different configurations. (handshake -s for example)
Sashan
force-pushed
the
run-perf.sh
branch
4 times, most recently
from
bde776f to
9dd36d5
Compare
|
new version, ready for review. list of changes is as follows:
below is sample output. I used modified version for 8 and 128 threads using 2 iterations so I don't wait for ages. evp_fetch
randbytes
handshake
sslnew
newrawkey
rsasign
x509storeissuer
providerdoall
rwlocks-rlock
rwlocks-wlock
pkeyread-dh-der
pkeyread-dhx-der
pkeyread-dsa-der
pkeyread-ec-der
pkeyread-rsa-der
pkeyread-x25519-der
pkeyread-dh-pem
pkeyread-dhx-pem
pkeyread-dsa-pem
pkeyread-ec-pem
pkeyread-rsa-pem
pkeyread-x25519-pem
_fetch
randbytes
handshake
sslnew
newrawkey
rsasign
x509storeissuer
providerdoall
rwlocks-rlock
rwlocks-wlock
pkeyread-dh-der
pkeyread-dhx-der
pkeyread-dsa-der
pkeyread-ec-der
pkeyread-rsa-der
pkeyread-x25519-der
pkeyread-dh-pem
pkeyread-dhx-pem
pkeyread-dsa-pem
pkeyread-ec-pem
pkeyread-rsa-pem
pkeyread-x25519-pem
|
Sashan
force-pushed
the
run-perf.sh
branch
2 times, most recently
from
286fcfe to
198480b
Compare
for OpenSSL versions 1.1.1 3.0 and master. It collects average time each single tool took to run in 25 iterations. The script produces results/report.md.
… for OpenSSL versions 1.1.1 3.0 and master. It collects average time each single tool took to run in 25 iterations. The script produces results/report.md.
…l tools for OpenSSL versions 1.1.1 3.0 and master. It collects average time each single tool took to run in 25 iterations. The script produces results/report.md.
|
closing this pull request in favour of #208 with improvements by @vdukhovni |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
for OpenSSL versions 1.1.1 3.0 and master. It collects average time each single tool took to run in 25 iterations. The script produces results/report.md.