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

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

Closed
wants to merge 1 commit into from

Conversation

Sashan
Copy link
Contributor

@Sashan Sashan commented Jun 19, 2024

…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.

@t8m
Copy link
Member

t8m commented Jun 19, 2024

@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.

@Sashan
Copy link
Contributor Author

Sashan commented Jun 19, 2024

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.

@t8m
Copy link
Member

t8m commented Jun 19, 2024

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.

…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.
@paulidale
Copy link
Contributor

Agreed @t8m, options are better here.

@Sashan
Copy link
Contributor Author

Sashan commented Jun 20, 2024

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.

@t8m
Copy link
Member

t8m commented Jun 20, 2024

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.

@Sashan
Copy link
Contributor Author

Sashan commented Jun 20, 2024

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.

@paulidale
Copy link
Contributor

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.

Sashan added a commit to Sashan/tools that referenced this pull request Jun 21, 2024
of implicit arguments (symlinks to tools). The approach is discussed
in those two PRs:
	openssl#205
	openssl#203
@t8m
Copy link
Member

t8m commented Aug 16, 2024

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

@t8m t8m closed this Aug 16, 2024
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

3 participants