Skip to content

Conversation

@krzikalla
Copy link
Contributor

As requested, here comes a new pull request to enable the user to perform multi-threading benchmarks using other threading APIs.

This one takes another approach, which requires only a few changes in the existing benchmark code.
Personally I like this approach the most, as it decouples the actual benchmark code and the user threading code. In addition, there is no special thread_state anymore.

Remark: line 305 of benchmark_runner.cc: manager->WaitForAllThreads(); has been removed. I don't think, that this causes any issues, as all threads join directly afterwards (all tests run fine). In fact I think, that some more things regarding threads in benchmark_runner.cc could be refactored. E.g. I don't understand this code:

  // Acquire the measurements/counters from the manager, UNDER THE LOCK!
  {
    MutexLock l(manager->GetBenchmarkMutex());
    i.results = manager->results;
  }

IMHO the mutex is not needed there, as all spawned threads have already been joined.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@LebedevRI
Copy link
Collaborator

Ok, now that's completely different from the original PR.
This is very straight-forward and reasonable. I like this.

What about the second part of that PR, ManualThreading()?
I think that feature is useful too, but like i said,
let's have separate PR's for these.

dmah42
dmah42 previously approved these changes Mar 26, 2025
@krzikalla
Copy link
Contributor Author

Does anyone know, what these failed windows tests are all about?
Some windows tests run successfully, others exit with exit code 0xc0000135 (which is related to the .NET framework(???)).
Then again, when a run fails, it fails at my newly introduced test (manual_threading_test), so it somehow seems to be related to my changes.

@LebedevRI
Copy link
Collaborator

Rebase pls

krzikalla and others added 7 commits March 27, 2025 16:20
Support the benchmarking of code, which relies on other
multi-threading APIs, e.g. OpenMP.
Support the benchmarking of code, which relies on other
multi-threading APIs, e.g. OpenMP.
* Manual thread runner test tests for actual invocation.
@LebedevRI
Copy link
Collaborator

I'm not sure what is going on with those failed builds,
but i do note that only -DBUILD_SHARED_LIBS=ON ones are failing,
so there must be something funny going on with symbol visibility.

@LebedevRI
Copy link
Collaborator

Awesome. I will need to sleep on this before doing further review passes, but this does look reasonable.

@dmah42
Copy link
Member

dmah42 commented Mar 28, 2025

i'm happy with this but i'll let @LebedevRI make the final decision and merge :)

thank you so much @krzikalla

Copy link
Collaborator

@LebedevRI LebedevRI left a comment

Choose a reason for hiding this comment

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

This is probably fine.

Test is rather too bare-bones,
perhaps we should be checking the output?

@LebedevRI LebedevRI merged commit 0da57b8 into google:main Mar 29, 2025
86 checks passed
@LebedevRI
Copy link
Collaborator

@krzikalla thank you!

Any thoughts on the second part of the original PR?

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