search

bheisler / criterion.rs

Statistics-driven benchmarking library for Rust
Apache License 2.0
4.52k stars 301 forks source link

Reexport std::hint::black_box #701

Closed Spartan2909 closed 2 months ago

Spartan2909 commented 1 year ago

Fixes #700.

waywardmonkeys commented 1 year ago

Would it make sense to possibly have a black_box function still, but mark it as deprecated and suggest calling std::hint::black_box instead, so that people can migrate?

Spartan2909 commented 1 year ago

That's also a good option. @bheisler any opinions?

lemmih commented 1 year ago

That's also a good option. @bheisler any opinions?

I think that sounds like a good idea.

bheisler commented 1 year ago

Oh, wow. They finally stabilized std::hint::black_box? I didn't think that would ever happen.

Yeah, deprecating our hacky replacement for black_box seems good to me, but keep in mind the minimum-supported-Rust-version policy. It wouldn't seem right to deprecate in favor of a stdlib function that hasn't existed for at least three major versions.

waywardmonkeys commented 1 year ago

Stable (const unstable) since 1.66 ... so that's 5 versions now if I count correctly. :)

Spartan2909 commented 1 year ago

I've added a deprecated wrapper for std::hint::black_box.

waywardmonkeys commented 1 year ago

You will probably need to bump the MSRV in CI and elsewhere since it is currently stating and testing 1.64

waywardmonkeys commented 1 year ago

Alternatively, deprecate in favor of std::hint but keep the old implementation for those that don’t want to update? Up to the maintainers!

d-e-s-o commented 1 year ago

Any chance the real_blackbox feature could be cleaned up in the process? Or is this a compat hazard?

Spartan2909 commented 1 year ago

I've bumped the MSRV and removed the use of the test feature. @d-e-s-o removing a feature is a breaking change, but could be done with a minor version bump as Criterion is still <1.0. I'll leave this up to the maintainers.

lilyball commented 1 year ago

I would really love to see this merged and a new release published. asan keeps sporadically triggering a false positive on the current black_box impl when used with zero-sized types so this change is more than just a performance fix.

Spartan2909 commented 4 months ago

@bheisler Is there anything blocking this?

waywardmonkeys commented 2 months ago

@lemmih This can be closed now that #794 has landed. Thanks to @Spartan2909 for the work done here as helped pave the way!