Skip to content

Make our handshake results more intuitive #29

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Make our handshake results more intuitive #29

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions source/handshake.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ void usage(const char *progname)
int main(int argc, char * const argv[])
{
double persec;
OSSL_TIME duration, ttime;
OSSL_TIME duration;
double avcalltime;
int ret = EXIT_FAILURE;
int i;
Expand Down Expand Up @@ -396,13 +396,14 @@ int main(int argc, char * const argv[])
goto err;
}

ttime = times[0];
for (i = 1; i < threadcount; i++)
ttime = ossl_time_add(ttime, times[i]);
avcalltime = (double)0;
persec += (double)0;
for (i = 0; i < threadcount; i++) {
avcalltime += ((double)ossl_time2ticks(times[i]) / (num_calls / threadcount)) / (double)OSSL_TIME_US;
persec += (((num_calls * OSSL_TIME_SECOND) / (double)threadcount) / (double)ossl_time2ticks(times[i]));
Comment on lines -399 to +403
Copy link
Member

@t8m t8m Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I do not think this is a correct change. Either we want to compute the wall clock time of all the threads spent in the computation or we want to compute how much time it took to perform the num_calls operations regardless of the number of threads involved. This is something in the middle that has no real meaning IMO.

Furthermore we will loose consistency in the graphs tracking the library changes over time.

I instead propose to use the existing persec number (which measures the number of executions per second for the whole process) for comparison across number of threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can accept a different change, in that it makes more intuitive sense to me to compute stats using wall clock time rather than per-thread time, as that really makes more intuitive sense to me, as that is going to be representative of what an application sees overall.

I disagree that the current persec (handshakes per seconds number) accurately measures the number of executions for the process, as we are assuming the process time is the cumulative time spent in the process is the sum of each threads execution time (i.e. 2 threads running for 2 second of wall clock time on 2 cpus is calculated as 4 total seconds, which stands in stark contrast to the wall clock run time of a process, which would be 2 seconds. This becomes quite attenuated at higher thread counts.

Insofar as consistency is concerned, I get that, but I feel like the implication there is that, even if the stats are not useful, we can't change them, which seems to make it impossible to fix/change anything.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I can accept that we should change all tests to use wall clock time of the process instead of this sum of wall clock times of all threads. But we need to at least do it consistently across all the test apps. And if we are changing it we should IMO also change the tested number of threads because the current 1, 10, 100, 500 does not give enough granularity where the difference between 100 and 500 does not give much more information.

Something like 1, 4, 16, 64, 256 would be IMO much better for logarithmic scale graphs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, makes sense. I'll close this in favor of a new pr that addresses all tests shortly.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we need more steps than 1, 10, 100 and 500. The proposal to increase by a factor of 4 works for me, but I think I prefer a factor of 2. I don't see the point of logarithmic scale graphs, so anything with more points in between works for me. I also wondering if there is a point in testing this with more threads than the available number of cores.

}

avcalltime = ((double)ossl_time2ticks(ttime) / num_calls) / (double)OSSL_TIME_US;
persec = ((num_calls * OSSL_TIME_SECOND)
/ (double)ossl_time2ticks(duration));
avcalltime /= (double)threadcount;

if (terse) {
printf("%lf\n", avcalltime);
Expand Down