Skip to content

Conversation

@esyr
Copy link
Member

@esyr esyr commented Nov 11, 2025

As max_time was initialised only once, the runs after the first one terminated immediately. Move max_time inside the key type loop.

Fixes: 7388d62 "Set a constant amount of runtime on more perf tests"

As max_time was initialised only once, the runs after the first one
terminated immediately.  Move max_time inside the key type loop.

Fixes: 7388d62 "Set a constant amount of runtime on more perf tests"
Signed-off-by: Eugene Syromiatnikov <[email protected]>
@esyr esyr requested review from andrewkdinh and jogme and removed request for andrewkdinh November 11, 2025 13:52
Copy link
Contributor

@jogme jogme left a comment

Choose a reason for hiding this comment

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

At many (if not all) places the max time is not set directly before calling run_tests. I'm not sure if that's a feature or a bug, but if the latter it would be worth fixing through the codebase

@esyr
Copy link
Member Author

esyr commented Nov 11, 2025

Well, the only other place where it induced a bug has been already fixed in e5cd2d4, and touching it just to get the proximity seems superficial to me; if anything, the whole test harness needs to be factored out, to avoid such bugs altogether.

@jogme
Copy link
Contributor

jogme commented Nov 11, 2025

If this specifically solves a bug then MB.

I was nitpicking about the fact, that we are doing operations such as memory allocation in the measured section, which if not explicitly intended then it's not correct IMO. Also it's probably not such an overload, but it's picking my OCD

@esyr
Copy link
Member Author

esyr commented Nov 11, 2025

There's a lot of that in terms of measurement: the fact that threads hammer the same counts array and constantly invalidate the cache line, the ossl_time_now() call on each iteration, inclusion of thread creation in the measured time, the list goes on; but, well, one thing at a time.

Copy link
Contributor

@jogme jogme left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@andrewkdinh andrewkdinh left a comment

Choose a reason for hiding this comment

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

Ah, my bad.

@quarckster fyi this might affect evp_setpeer perf test

Edit: Oh actually, since this is just for -k all then it probably won't

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.

3 participants