[benchmark_harness] Add blackhole#2410
Conversation
Give benchmark writers a place to put results that help avoid compiler optimizations that can skew results
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with
Coverage
|
| File | Coverage |
|---|---|
| pkgs/benchmark_harness/lib/benchmark_harness.dart | 💔 Not covered |
| pkgs/benchmark_harness/lib/src/async_benchmark_base.dart | 💚 88 % ⬆️ 1 % |
| pkgs/benchmark_harness/lib/src/benchmark_base.dart | 💔 81 % ⬇️ 4 % |
| pkgs/benchmark_harness/lib/src/blackhole.dart | 💚 75 % |
| pkgs/benchmark_harness/lib/src/perf_benchmark_base.dart | 💔 Not covered |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check.
API leaks ✔️
The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
| Package | Leaked API symbol | Leaking sources |
|---|
This check can be disabled by tagging the PR with skip-leaking-check.
Unused Dependencies ⚠️
| Package | Status |
|---|---|
| benchmark_harness | ❗ Show IssuesThese packages may be unused, or you may be using assets from these packages: |
For details on how to fix these, see dependency_validator.
This check can be disabled by tagging the PR with skip-unused-dependencies-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
This check can be disabled by tagging the PR with skip-changelog-check.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a blackhole utility to the benchmark_harness package to prevent dead-code elimination (DCE) in microbenchmarks. It includes the implementation of the Blackhole class, integration into sync and async benchmark bases, and comprehensive tests. Review feedback focuses on ensuring the effectiveness of the DCE prevention by moving preventDCE() calls to after the measurement loops and refining the Blackhole class implementation for better type safety and reduced redundancy.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a blackhole utility to prevent dead-code elimination in microbenchmarks, integrating it into the measurement loops of BenchmarkBase, AsyncBenchmarkBase, and PerfBenchmarkBase. The reviewer recommends refactoring the Blackhole class into top-level functions to align with Effective Dart guidelines and suggests using a more robust opaque condition to ensure the utility remains effective against modern compiler optimizations.
| class Blackhole { | ||
| static Object? _sink; | ||
|
|
||
| /// An opaque guard that convinces compiler static analyses (such as TFA) | ||
| /// that [_sink] is read, preventing it from being tree-shaken as write-only. | ||
| /// | ||
| /// Automatically invoked inside benchmark measurement loops. | ||
| @pragma('vm:never-inline') | ||
| @pragma('dart2js:never-inline') | ||
| @pragma('wasm:never-inline') | ||
| static void preventDCE() { | ||
| // Opaque condition that is always false at runtime but unresolvable | ||
| // at compile-time. | ||
| if (int.tryParse('0') == 1) { | ||
| print(_sink); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A zero-cost compiler-safe live sink to prevent dead-code elimination. | ||
| @pragma('vm:prefer-inline') | ||
| @pragma('dart2js:prefer-inline') | ||
| @pragma('wasm:prefer-inline') | ||
| void blackhole(Object? value) { | ||
| Blackhole._sink = value; | ||
| } |
There was a problem hiding this comment.
In Dart, it is more idiomatic to use top-level functions and variables instead of a class that contains only static members. This follows the Effective Dart recommendation. Additionally, the opaque condition int.tryParse('0') == 1 might be constant-folded by modern compilers; using something like DateTime.now().year < 0 would be more robust for preventing dead-code elimination across different platforms.
Object? _sink;
/// An opaque guard that convinces compiler static analyses (such as TFA)
/// that [_sink] is read, preventing it from being tree-shaken as write-only.
///
/// Automatically invoked inside benchmark measurement loops.
@pragma('vm:never-inline')
@pragma('dart2js:never-inline')
@pragma('wasm:never-inline')
void preventDCE() {
// Opaque condition that is always false at runtime but unresolvable
// at compile-time.
if (DateTime.now().year < 0) {
print(_sink);
}
}
/// A zero-cost compiler-safe live sink to prevent dead-code elimination.
@pragma('vm:prefer-inline')
@pragma('dart2js:prefer-inline')
@pragma('wasm:prefer-inline')
void blackhole(Object? value) {
_sink = value;
}References
- AVOID defining a class that contains only static members. (link)
- Prioritize robust, future-proof implementations for foundational components to avoid issues like dead-code elimination.
| } | ||
| return elapsed / iter; | ||
| } finally { | ||
| Blackhole.preventDCE(); |
There was a problem hiding this comment.
Call the top-level preventDCE() function directly instead of using the static class member.
| Blackhole.preventDCE(); | |
| preventDCE(); |
| return await measureFor(exercise, 2000); | ||
| } finally { | ||
| await teardown(); | ||
| Blackhole.preventDCE(); |
There was a problem hiding this comment.
Call the top-level preventDCE() function directly instead of using the static class member.
| Blackhole.preventDCE(); | |
| preventDCE(); |
| try { | ||
| return measureForImpl(f, minimumMillis).score; | ||
| } finally { | ||
| Blackhole.preventDCE(); |
There was a problem hiding this comment.
Call the top-level preventDCE() function directly instead of using the static class member.
| Blackhole.preventDCE(); | |
| preventDCE(); |
| return result.score; | ||
| } finally { | ||
| teardown(); | ||
| Blackhole.preventDCE(); |
There was a problem hiding this comment.
Call the top-level preventDCE() function directly instead of using the static class member.
| Blackhole.preventDCE(); | |
| preventDCE(); |
| } | ||
| } finally { | ||
| teardown(); | ||
| Blackhole.preventDCE(); |
There was a problem hiding this comment.
Call the top-level preventDCE() function directly instead of using the static class member.
| Blackhole.preventDCE(); | |
| preventDCE(); |
| }); | ||
| }); | ||
|
|
||
| group('Blackhole class preventDCE', () { |
There was a problem hiding this comment.
Update the group name to reflect the move to a top-level function.
| group('Blackhole class preventDCE', () { | |
| group('preventDCE', () { |
|
|
||
| group('Blackhole class preventDCE', () { | ||
| test('preventDCE executes successfully without throwing errors', () { | ||
| expect(Blackhole.preventDCE, returnsNormally); |
There was a problem hiding this comment.
Test the top-level preventDCE() function directly.
| expect(Blackhole.preventDCE, returnsNormally); | |
| expect(preventDCE, returnsNormally); |
| static void preventDCE() { | ||
| // Opaque condition that is always false at runtime but unresolvable | ||
| // at compile-time. | ||
| if (int.tryParse('0') == 1) { |
There was a problem hiding this comment.
Don't assume compilers can't optimize int.parse.
Consider if (DateTime.now().millisecondsSinceEpoch < 0).
That should not ever be compile-time optimized.
| } | ||
| } | ||
|
|
||
| /// A zero-cost compiler-safe live sink to prevent dead-code elimination. |
There was a problem hiding this comment.
Why/how does it prevent dead-code elemination?
| iter++; | ||
| } | ||
| return elapsed / iter; | ||
| } finally { |
There was a problem hiding this comment.
Putting code inside a try/finally can affect compilation. I wouldn't do that in a benchmark.
If anything, put it in a catch clause and before the return. finally is odd to optimize.
| } | ||
| return elapsed / iter; | ||
| } finally { | ||
| Blackhole.preventDCE(); |
There was a problem hiding this comment.
This does not depend on anything inside the loop, so a compiler can still eliminate the loop content.
If the post-benchmark code does not depend on the output of f(), and f doesn't have visible side effects (which none of the benchmarked functions in a function likely has), then the compiler can just not call f() more than once (just to see if it throws).
Don't assume compilers are stupid. That can always change in the future.
2 participants
Give benchmark writers a place to put results that help avoid compiler optimizations that can skew results