Separate benchmarks by enabled features

[ivarflakstad]

With this results from --features=metal won't interfere with results from --features=accelerate etc.

> cargo bench --features=metal

metal_matmul/iter       time:   [73.133 µs 73.209 µs 73.301 µs]
                        thrpt:  [53.291 GiB/s 53.358 GiB/s 53.413 GiB/s]
                 change:
                        time:   [-5.0137% -1.8311% +1.3288%] (p = 0.29 > 0.05)
                        thrpt:  [-1.3114% +1.8652% +5.2783%]
                        No change in performance detected.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) high mild
  7 (7.00%) high severe

> cargo bench --features=accelerate

accelerate_matmul/iter  time:   [319.22 µs 325.10 µs 332.30 µs]
                        thrpt:  [11.755 GiB/s 12.016 GiB/s 12.237 GiB/s]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild

[LaurentMazare]

Do we really need this allow pragma? Maybe there is a way to avoid it.

[ivarflakstad]

I agree, but I don't know a way to get around it. Criterion doesn't run the code in the same way as the standard rust testing framework, so it doesn't register as being used.

[LaurentMazare]

Ok I guess you googled a bit on the criterion side and haven't found anything (I haven't checked), could you add a comment to explain why these are needed? (and link to any criterion resource that would document this if any)

[ivarflakstad]

Ok, cargo isn't complaining about unused code anymore 👍

[LaurentMazare]

Are you sure? I just tried running the benchmarks with no feature enabled and got the warnings?

[ivarflakstad]

Hmm. I tried after cargo clean and still get no warnings 🤔

[kyle-mccarthy]

I think this is related to the /benches folder file structure https://users.rust-lang.org/t/how-to-properly-add-test-cases-for-benches/102859/4

[ivarflakstad]

Are the warnings still present? If they are still there for any of you I'll try a different approach.

[Narsil]

Yes. Just remove the utils file. Bench will walk all files including the utils which is why it will complain. Just but them in the core file.

[ivarflakstad]

By "core" file, do you mean matmul.rs?
I was thinking we could have several benchmarks matmul.rs, random.rs, etc.
If I place these in their own module and load them from a bench_main.rs file we won't be able to specify which benchmark to run.

[Narsil]

I'm wondering why we don't have both benches when compiling with --features metal.

Is it hard to change it so we can have both (we can skip the bench on metal when metal is not active, but not having CPU bench when metal is active feels odd)

[Narsil]

Could we derive this name from the actual device, not do guesswork on the features enabled (keep the guessing for MKL/Accelerate I don't know how to do it for those).

[ivarflakstad]

It is the exact same guesswork as how I obtain the actual device, so technically it won't be less guessy.

pub(crate) fn device() -> Result<Device> {
    if cfg!(feature = "metal") {
        Device::new_metal(0)
    } else if cfg!(feature = "cuda") {
        Device::new_cuda(0)
    } else {
        Ok(Device::Cpu)
    }
}

It does make it more consistent though.

[ivarflakstad]

Updated now 😊

[ivarflakstad]

we can skip the bench on metal when metal is not active, but not having CPU bench when metal is active feels odd

Ah, I think I understand.
If there are changes to rust code that slows down the GPU benchmarks you could be misled to believe it is because of the GPU kernels. If we have CPU benchmarks as a baseline you'll immediately see that it is not related.
Did I understand correctly?

I'll look into it.

[Narsil]

we can skip the bench on metal when metal is not active, but not having CPU bench when metal is active feels odd

Ah, I think I understand. If there are changes to rust code that slows down the GPU benchmarks you could be misled to believe it is because of the GPU kernels. If we have CPU benchmarks as a baseline you'll immediately see that it is not related. Did I understand correctly?

I'll look into it.

It's more about consistency. In general features should be additive. This is not the case for accelerate and mkl but it should be the case for metal and cuda since they only "add" the other backends. (candle-examples can be a bit more liberal since it's a showcase, not the actual library).

The tests also follow that behavior.

[ivarflakstad]

It's more about consistency. In general features should be additive.

For kernels it's generally speaking closer to replacing certain parts than adding new ones, is it not?

[Narsil]

For kernels it's generally speaking closer to replacing certain parts than adding new ones, is it not?

No it's not. adding the metal features, enables a user to use the Device::Metal. You can still use CPU if you so desire.

[ivarflakstad]

No it's not. adding the metal features, enables a user to use the Device::Metal. You can still use CPU if you so desire.

Right you are! 😊 Updated the code.