tools icon indicating copy to clipboard operation
tools copied to clipboard

This change adds rwlocks.rlock and rwlocks.wlock symlinks rwlocks bin…

Open Sashan opened this issue 1 year ago • 8 comments

…ary.

We need it so we can pass 'implicit' option so we can chose which statistics want to collect. This helps us with our current simple tooling we have to collect stats. The script we currently work on expects tool prints a single number as its output.

Sashan avatar Jun 19 '24 05:06 Sashan

@Sashan please reconsider this approach. IMO it is not scaleable. IMO we need to be able to pass some arbitrary options from the statistics collection script and be able to invoke the same perf tool with different options.

t8m avatar Jun 19 '24 11:06 t8m

I agree it's not scalable. On the other hand it is good enough for what we have now. We just glue it with script I got from Niel collect the results almost automatically and be done with it for 3.4. We can then revisit it later.

In this particular case the rwlocks tool reports two lines of output (time for r-lock, time for w-lock), while other tools typically report single number which we can just grab and put it to report. The report can be whatever format we need. I currently use markdown, but it can be json which we can feed to graphana.

It's true we can make the script which drives performance tests more complex so it will be able to deal with multiline output but this really clobbers the logic in loop which executes the individual tools. Right now I'm looking for some simple and quick solution which will get us going. We can always revisit later, in my opinion.

Sashan avatar Jun 19 '24 12:06 Sashan

I have no problem with adding an option for limiting the output to just one number. What I have a problem with is the creation of the duplicate binaries (or symlinking) instead of having a possibility to specify extra arguments in the statistics collection script.

t8m avatar Jun 19 '24 15:06 t8m

Agreed @t8m, options are better here.

paulidale avatar Jun 20 '24 02:06 paulidale

Agreed @t8m, options are better here.

I disagree. Please refer to script run-perf.sh which I"ve just submitted in PR here: https://github.com/openssl/tools/pull/207/files

The extra/special options would ask us to add logic into the loop which runs all tools for all library versions. In my opinion we are winning if we don't need those special options. I see this way as good enough for now so we can use the same script to collect data we need for unix-like systems.

And I agree the approach has limits in longrun, but I think we can deal with those later as our test performance infrastructure will grow. Right now I'd like to start with something minimalistic just to start moving.

Sashan avatar Jun 20 '24 07:06 Sashan

I think adding an explicit list of commands with arguments into the perf.sh script instead just list of commands is nothing that would complicate the run-perf.sh script in any serious way.

t8m avatar Jun 20 '24 09:06 t8m

I think adding an explicit list of commands with arguments into the perf.sh script instead just list of commands is nothing that would complicate the run-perf.sh script in any serious way.

the reason why I persist on the idea of implicit options (at least for now) is that we can use wildcards to selectively chose tests to run. for example if we creat yeat-another custom script to get numbers quickly for rwlocks and pkeyread in ber format we just do: for TEST in rwlocks-* pkeyread-*-der; do $TEST ; done

if we will be using explicit options to rwlocks then it won't be that straightforward I think.

I'm also not saying the implicit options are perfect or ultimate thing we should keep forever. I see it as good enough for now just to get going. At this point we have a simple script which seems to be doing its job. We can always go for more fancy solution with explicit options later.

but if you really insist I can give it a try and craft run-perf.sh which will be using explicit options.

Sashan avatar Jun 20 '24 09:06 Sashan

I also prefer doing this using arguments. What's here is Unix specific which precludes running these performance tests on non-Unixy platforms.

I appreciate the wildcard concern but don't find it compelling.

paulidale avatar Jun 20 '24 22:06 paulidale

Please move this PR to perftools repo if it is still relevant.

t8m avatar Aug 16 '24 08:08 t8m