Skip to content

Conversation

@loonatick-src
Copy link
Contributor

@loonatick-src loonatick-src commented Apr 30, 2025

Description

This PR adds another optimization blocker function to Blackhole.swift. It could be thought of as the analogue of Google benchmark's benchmark::DoNotOptimize.

@_optimize(none)
func blackHoleMutating(_: UnsafeMutableRawPointer) {}

This forces the compiler to assume that the passed argument is mutated in addition to just read. Consequently it can be used to block optimizations like loop-invariant code motion (LICM) that could happen when using an amortizing inner loop using benchmark.scaledIterations. An example is provided in the doc comment.

We had to implement this for some of our benchmarks, and it could potentially be useful for other users of package-benchmark as well.

It would be convenient to have it in package-benchmark as it appears that @_optimize(none) is expected to work only if it's used in a different module.

How Has This Been Tested?

I could use some guidelines on how to add a test for something like this.

It has not been "unit-tested" per se, but we have been using it in our benchmarks as mentioned above. The compiler was able to completely const-fold some of those loops without the clobber (this PR's blackHoleMutating).

There are some disabled tests in AdditionalTests.swift that include a test for blackHole. Perhaps a test could be added over there if those tests are enabled again. It appears that the tests could be enabled now as the issue mentioned therein (#178) is closed as completed now?

Minimal checklist:

  • I have performed a self-review of my own code
  • I have added DocC code-level documentation for any public interfaces exported by the package
  • I have added unit and/or integration tests that prove my fix is effective or that my feature works

@codecov
Copy link

codecov bot commented Apr 30, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.46%. Comparing base (3db567f) to head (bd5f48a).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
Sources/Benchmark/Blackhole.swift 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
- Coverage   69.48%   69.46%   -0.02%     
==========================================
  Files          33       33              
  Lines        3938     3939       +1     
==========================================
  Hits         2736     2736              
- Misses       1202     1203       +1     
Files with missing lines Coverage Δ
Sources/Benchmark/Blackhole.swift 20.00% <0.00%> (-5.00%) ⬇️
Files with missing lines Coverage Δ
Sources/Benchmark/Blackhole.swift 20.00% <0.00%> (-5.00%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c4569a...bd5f48a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hassila
Copy link
Contributor

hassila commented May 3, 2025

I think this is a great addition, very happy to take it, thanks! WDYT choosing something like blackHoleMutating instead of clobber (which I personally feels is the naming of a function that truly would clobber/overwrite stuff, not just "fake it").

@hassila
Copy link
Contributor

hassila commented May 3, 2025

Probably should add a sample of using this / not using this into the test benchmarks embedded in the project, to highlight the importance and differences it makes?

@loonatick-src
Copy link
Contributor Author

I think this is a great addition, very happy to take it, thanks! WDYT choosing something like blackHoleMutating instead of clobber (which I personally feels is the naming of a function that truly would clobber/overwrite stuff, not just "fake it").

Makes sense. I'll do that along with example usage this week.

@loonatick-src loonatick-src marked this pull request as draft May 15, 2025 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants