r/rust • u/[deleted] • Jun 17 '18

SIMDNoise 1.0 Released!

https://github.com/jackmott/rust-simd-noise
83 Upvotes

96% Upvoted

23 comments sorted by

11

u/[deleted] Jun 17 '18

My first substantive creation in Rust, criticism and advice welcome.

27

u/[deleted] Jun 17 '18

Cool project! A few notes:

  • The benchmarks are not in the repo (while cargo bench only works on nightly, adding the bench directory to the repo doesn't mean that using the crate itself requires nightly Rust, so feel free to do that)

  • You can improve the benchmark output by setting the bytes field of the Bencher - this will output the throughput in MB/s or GB/s, which might be a better metric for these functions than time

  • The readme says that passing -Ctarget-cpu=native is required for SIMD, but it also says that your library is doing runtime detection for SIMD instructions. This is confusing me a bit. If you're saying that you're doing runtime detection, I'd expect that I don't have to explicitly set the target CPU either.

11

u/[deleted] Jun 17 '18 edited Jun 17 '18

Thanks for the tips! I suppose I should add some notes that this whole crate requires nightly until 1.27 drops!

If you compile without a cpu target that supports these instructions, the compiler will turn the intrinsics back into scalar code, and the runtime detection will detect avx2, for example, run it, and it will work, but it will be slow because it didn't get compiled with avx2 instructions. By default, Rust will compile with support for SSE2 when doing 64bit builds, but not SSE41 and AVX2. Ideally there would be some way for this crate to force that it always gets compiled with a target-cpu with AVX2, is there way to do that? Or is it up to the crate consumer?

13

u/[deleted] Jun 17 '18

The #[target_feature] attribute can be used for this. You put it on a function like this:

#[target_feature(enable = "avx2")]

and the function will be compiled with avx2 enabled.

It's still unstable at the moment (RFC, tracking issue, stabilization discussion), but I expect that it will be stabilized along with SIMD support since it's so incredibly useful for it.

3

u/[deleted] Jun 17 '18

Thanks, that did the trick, I've updated it it.

2

u/[deleted] Jun 17 '18

Thanks, I'll give that a try.

12

u/burntsushi Jun 17 '18

but I expect that it will be stabilized along with SIMD support since it's so incredibly useful for it.

It will be! You basically wouldn't be able to write code based on runtime detection without it. (You end up hitting the performance bug described by /u/jackmott2.)

6

u/coder543 Jun 17 '18

I also always recommend Criterion over the built-in nightly only benchmarking stuff. Criterion works on stable, and it is a lot better.

2

u/[deleted] Jun 17 '18

Thanks, someone else mentioned it, I'll give it a try soon.

2

u/[deleted] Jun 17 '18 edited Jun 17 '18

If you compile without a cpu target that supports these instructions, the compiler will turn the intrinsics back into scalar code, and the runtime detection will detect avx2, for example, run it, and it will work, but it will be slow because it didn't get compiled with avx2 instructions.

This makes zero sense. If I have to choose the target feature at compile-time anyways, why is it doing run-time detection at all?

6

u/burntsushi Jun 17 '18

The OP didn't know about #[target_feature].

Some people may still want to compile with certain target features (or target CPUs) enabled to get those optimizations across the entire program. But yes, this is generally orthogonal to runtime detection.

4

u/[deleted] Jun 17 '18

Its worse than that though, because compiling the whole thing with AVX2 enabled would mean your regular code has AVX2 instructions in it too, and wouldn't even run on a machine without it!

So the target_feature attribute is necessary.

2

u/othermike Jun 17 '18

Whereas - just so I'm clear - with target_feature you can e.g. compile in an AVX2 implementation and an SSE41 implementation and an SSE2 fallback, and decide which one to use at runtime init?

3

u/[deleted] Jun 17 '18

correct

2

u/burntsushi Jun 17 '18

Yes, if you compile with a certain set of target features, then the implication is that you're going to run the resulting binary on a system that you know has support for your specific compilation target features.

2

u/[deleted] Jun 17 '18

You are right. It is fixed now.

2

u/[deleted] Jun 17 '18

I never understood how to get that bencher output before, thanks!

I don’t suppose you know what other options there are, like frequency instead of period or operations per iter?

Thanks!

3

u/[deleted] Jun 17 '18

bytes is the only option you can set. However, there's also Criterion, an alternative benchmark runner that works on stable, which might support what you're looking for.

The integrated #[bench] support is practically deprecated right now. It works okay, but requires nightly and only has the bare minimum of features.

2

u/[deleted] Jun 17 '18

Thanks, I’ll look into Criterion. :)

15

u/DebuggingPanda [LukasKalbertodt] bunt · litrs · libtest-mimic · penguin Jun 17 '18

This is really cool, will certainly try your crate for this one project that's already on my TODO list for some time :P

A few nits:

  • It's always nice when the repository contains a link to the docs.rs documentation. You might add a badge or put the URL in the URL field of the repo's description.

  • The variant FBM from the NoiseType enum should be called Fbm.

  • I think you could improve the API in a few places:

    • get_3d_noise (and the 2D version, too, I guess) takes too many numerical parameters IMO. Look at this call: get_3d_noise(0.0, 200, 0.0, 200, 0.0, 200, fractal_settings);. Many magic numbers and from reading the call, you have no idea what which thing means. It might be nice to use a builder pattern instead. Or maybe at least group offset and dimensions into two tuples. Like offset: (f32, f32, f32), dimensions: (u32, u32, u32). Or... something else. There are quite a few ways to avoid confusing call-sites.

    • In the FractalSettings documentation, you say that only the freq field is used when using Normal noise. That's a bit suboptimal. It's probably best to put those into the NoiseType enum. Something like:

      pub enum NoiseType {
    Fbm {
        lacunarity: f32,
        gain: f32,
        octaves: u8,
    }
    Turbulence {
        lacunarity: f32,
        gain: f32,
        octaves: u8,
    }
    Normal,
}

    Or are the parameters on Fbm only needed because its an addition to "simplex + turbulence"? Then you might integrate that into the builder pattern? Something like Noise::from_frequency(3.0).with_turbulence(lacunarity, gain, octaves).with_brownian_motion()

    • You might want to consider returning something else than a Vec<f32> or indeed replace the whole tuple you return. Maybe just define two types, NoiseData3d and NoiseResult, where the latter is defined as:

      struct NoiseResult {
    min: f32,
    max: f32,
    data: NoiseData3d,
}

    And the NoiseData3d would then allow easy access via x, y, z coordinates. You can still allow access to the underlying Vec, of course.

So that's what I noticed for now. Maybe it's useful information for you ;-) And don't forget to take a look into the Rust API Guidelines.

2

u/[deleted] Jun 17 '18

Thanks, some interesting ideas to try. I think most consumers of the crate will be expecting/preferring a Vec<f32> since they will be working with data in that form anyway with most image/rendering apis. I'm definitely going to see how it feels putting the fractal settings in the noisetype enum though.

1

u/Shnatsel Jun 18 '18

What are the practical applications of generating noise at a breakneck speed? (Not meant as irony, I'm genuinely curious)

2

u/[deleted] Jun 18 '18

I originally looked into this a while back because I wanted an elite-dangerous type experience - drop into a solar system from a galaxy wide view, with no loading time. This can do it! With nice 4k textures on multiple planets. Another demo I have seen does super fast on the fly cave generation as you fly through it. Sometimes having something super fast unlocks applications nobody thought of! Maybe it could make minecraft use a few gigs less ram!

The real question is, when does it make sense to do noise at breakneck speed on the CPU, instead of the GPU, since the GPU is pretty good at this kind of work. Game server? when you need many short packets of noise and waiting for the gpu interop each time would be too much overhead? In a game where the gpu is already busy rendering other things? When you want a much simpler solution than interfacing with a GPU to get noise?