Add SIMD flag

[Sytten]

Description of changes

• Started flagging SIMD dependencies, I know they have fallbacks but the language used in those crates makes me nervous (The runtime detection will be skipped if the fastest implementation is already available at compile-time)

If we agree this is a good idea, then I would add some CI checks to make sure we compile both variants.

If the feature system of rust was a bit better I could add only a simd feature, but there is currently no way to do that so I added -simd variants.

Checklist

• Created unit tests in tests/unit and/or in Rust for my feature if needed
• Ran make fix to format JS and apply Clippy auto fixes
• Made sure my code didn't add any additional warnings: make check
• Updated documentation if needed (API.md/README.md/Other)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[richarddavison]

I'm not sure why we would disable this? Most of the simd enabled crates have either runtime and or compile time checks. They should work on both non-simd and simd enabled CPUs (which a large amount of CPUs has except for some RISC which is a bit out of scope). If this becomes a requested feature we should maybe provide optional "non-simd" builds but for now we'd like to keep it as fast as possible :)

[Sytten]

My concern is really with The runtime detection will be skipped if the fastest implementation is already available at compile-time expressed by those crates. It is not great if you are compiling the crate in CI and shipping to a lot of users with very different hardware (this is what we do).
The difference in speed in my case is not worth the risk of it crashing on some on my user machines.
Since llrt_modules are supposed to be a bit more universal, I would still like to provide rust only dependencies, but worst case I can maintain a fork of llrt_utils.

[Sytten]

I asked my question to the lib author: Nugine/simd#44

[richarddavison]

Sorry I was a bit to quick on closing it. That makes sense. We can just enable it for LLRT executable and not for the libs. However, should SIMD be opt in or opt out? 🤔

[Sytten]

The author wrote some precisions that I find acceptable. I am still on the fence on what to do here, on one hand those SIMD libraries are not mainstream vs the hex, base64 ones. Most people won't have them in their lockfile. On the other hand it seems safe to use that library since it does provide a fallback.

It is usually currently accepted that the best practice for a library proposing SIMD is for it to allow users to opt-in, though I really don't care.

[richarddavison]

Let’s enable it as default and then users can opt out by disable default features. Also I suggest that we only have one SIMD flag instead of multiple. LMKWYT

[Sytten]

Ok will change it, the problem with one flag is that you can't enable dependencies based on another flag.
This is the comment I made here: rust-lang/cargo#5954 (comment)
I am open to suggestion, I did try to make that happen since it is annoying but I didnt find a solution.

[richarddavison]

Ok will change it, the problem with one flag is that you can't enable dependencies based on another flag. This is the comment I made here: rust-lang/cargo#5954 (comment) I am open to suggestion, I did try to make that happen since it is annoying but I didnt find a solution.

You don't have to right? They will be dead code since we have config blocks on non-simd alternatives

[Sytten]

The code is fine, the problem is enabling the dependency in Cargo.toml.
This is not possible:

[features]
default = ["encoding"]
encoding = []
# ... other features not related to SIMD
simd = []

[target.'cfg(all(feature = "simd",  feature = "encoding"))'.dependencies]
base64-simd = "0.8.0"

[target.'cfg(all(not(feature = "simd"),  feature = "encoding"))'.dependencies]
base64 = "0.22.0"

[richarddavison]

The code is fine, the problem is enabling the dependency in Cargo.toml. This is not possible:

[features]
default = ["encoding"]
encoding = []
# ... other features not related to SIMD
simd = []

[target.'cfg(all(feature = "simd",  feature = "encoding"))'.dependencies]
base64-simd = "0.8.0"

[target.'cfg(all(not(feature = "simd"),  feature = "encoding"))'.dependencies]
base64 = "0.22.0"

I see what you mean and as you mentioned it's a cargo limitation. Size wise it's fine but it will add to compile times. I'm a bit reluctant of having multiple dependencies doing the same thing but are there any other alternatives?

[Sytten]

Same. AFAIK the only alternative is what I have done currently with encoding-simd, its ugly but it works.

[Sytten]

I will implement the separate crate like discussed, I will bundle the asm with the simd feature. Crates like md-5 don't compile at all on windows when they use asm so this feature really is needed.