Simplifying backend selection #414
Comments
|
Any chance to make the crate use additive features? |
|
The best way to do that would probably be to get rid of feature-gating for Then the only backend feature left is |
so long as there's still an override / way to disable the detection! sometimes it's useful to be able to run tests for 32-bit platforms under x86_64 with the u32_backend, and where target detection is involved,
i was wondering whether y'all had considered refactoring the backends to traits w/ implementations so you could build the library with any / all enabled, then default based on runtime detection or availability? similar to the way allocators are implemented now. i looked at starting / PRing this briefly but, it would be a lot of work without knowing it'd be useful / desirable. |
FWIW, we used to have explicit features for this in the https://github.com/RustCrypto/elliptic-curves crates, but got rid of the because you can test 32-bit code on 64-bit Linux by compiling for
We have a lot of experience with runtime feature detection in @RustCrypto using the
That seems pretty onerous. Many, many types would need a generic parameter for the backend, and there would need to be different backends for e.g. base and scalar fields. I think it's a lot simpler to make backend selection as automatic as possible, keeping all the complexity hidden so people don't have to think about it. |
|
Now I remember the I still think it's best for it to employ autodetection, but unfortunately that does still leave two features which are mutually incompatible ( |
Then it should be a |
|
Yeah, that's what we've started doing in the @RustCrypto crates. We've had a few complaints that it's hard to configure |
As proposed in #414, this commit changes the backend selection approach. introspecting `target_pointer_width` to select `u32_backend` vs `u64_backend` (or `fiat_u32_backend`/`fiat_u64_backend` if the `fiat_backend` feature is enabled. This helps eliminate the use of non-additive features, and also the rather confusing errors that happen if multiple backends are selected (i.e. thousands of lines of rustc errors). The selection logic checks if `target_pointer_width = "64"` and uses the 64-bit backend, or falls back to the 32-bit backend otherwise. This means the crate will always have a valid backend regardless of the pointer width, although there may be odd edge cases for exotic platforms which would optimally use the 64-bit backend but have a non-"64" target pointer width for whatever reason. We can handle those cases as they come up.
As proposed in #414, this commit changes the backend selection approach, introspecting `target_pointer_width` to select `u32_backend` vs `u64_backend` (or `fiat_u32_backend`/`fiat_u64_backend` if the `fiat_backend` feature is enabled. This helps eliminate the use of non-additive features, and also the rather confusing errors that happen if multiple backends are selected (i.e. thousands of lines of rustc errors). The selection logic checks if `target_pointer_width = "64"` and uses the 64-bit backend, or falls back to the 32-bit backend otherwise. This means the crate will always have a valid backend regardless of the pointer width, although there may be odd edge cases for exotic platforms which would optimally use the 64-bit backend but have a non-"64" target pointer width for whatever reason. We can handle those cases as they come up.
|
PR with an initial minimal implementation here: #428 |
As proposed in #414, this commit changes the backend selection approach, introspecting `target_pointer_width` to select `u32_backend` vs `u64_backend` (or `fiat_u32_backend`/`fiat_u64_backend` if the `fiat_backend` feature is enabled). This helps eliminate the use of non-additive features, and also the rather confusing errors that happen if multiple backends are selected (i.e. thousands of lines of rustc errors). The selection logic checks if `target_pointer_width = "64"` and uses the 64-bit backend, or falls back to the 32-bit backend otherwise. This means the crate will always have a valid backend regardless of the pointer width, although there may be odd edge cases for exotic platforms which would optimally use the 64-bit backend but have a non-"64" target pointer width for whatever reason. We can handle those cases as they come up.
As proposed in #414, this commit changes the backend selection approach, introspecting `target_pointer_width` to select `u32_backend` vs `u64_backend` (or `fiat_u32_backend`/`fiat_u64_backend` if the `fiat_backend` feature is enabled). This helps eliminate the use of non-additive features, and also the rather confusing errors that happen if multiple backends are selected (i.e. thousands of lines of rustc errors). The selection logic checks if `target_pointer_width = "64"` and uses the 64-bit backend, or falls back to the 32-bit backend otherwise. This means the crate will always have a valid backend regardless of the pointer width, although there may be odd edge cases for exotic platforms which would optimally use the 64-bit backend but have a non-"64" target pointer width for whatever reason. We can handle those cases as they come up.
As proposed in #414, this commit changes the backend selection approach, introspecting `target_pointer_width` to select `u32_backend` vs `u64_backend` (or `fiat_u32_backend`/`fiat_u64_backend` if the `fiat_backend` feature is enabled). This helps eliminate the use of non-additive features, and also the rather confusing errors that happen if multiple backends are selected (i.e. thousands of lines of rustc errors). The selection logic checks if `target_pointer_width = "64"` and uses the 64-bit backend, or falls back to the 32-bit backend otherwise. This means the crate will always have a valid backend regardless of the pointer width, although there may be odd edge cases for exotic platforms which would optimally use the 64-bit backend but have a non-"64" target pointer width for whatever reason. We can handle those cases as they come up.
As proposed in #414, this commit changes the backend selection approach, introspecting `target_pointer_width` to select `u32_backend` vs `u64_backend` (or `fiat_u32_backend`/`fiat_u64_backend` if the `fiat_backend` feature is enabled). This helps eliminate the use of non-additive features, and also the rather confusing errors that happen if multiple backends are selected (i.e. thousands of lines of rustc errors). The selection logic checks if `target_pointer_width = "64"` and uses the 64-bit backend, or falls back to the 32-bit backend otherwise. This means the crate will always have a valid backend regardless of the pointer width, although there may be odd edge cases for exotic platforms which would optimally use the 64-bit backend but have a non-"64" target pointer width for whatever reason. We can handle those cases as they come up.
As proposed in #414, this commit changes the backend selection approach, introspecting `target_pointer_width` to select `u32_backend` vs `u64_backend` (or `fiat_u32_backend`/`fiat_u64_backend` if the `fiat_backend` feature is enabled). This helps eliminate the use of non-additive features, and also the rather confusing errors that happen if multiple backends are selected (i.e. thousands of lines of rustc errors). The selection logic checks if `target_pointer_width = "64"` and uses the 64-bit backend, or falls back to the 32-bit backend otherwise. This means the crate will always have a valid backend regardless of the pointer width, although there may be odd edge cases for exotic platforms which would optimally use the 64-bit backend but have a non-"64" target pointer width for whatever reason. We can handle those cases as they come up.
|
Question, now that I've been looking at docs: when documenting private items how should we document all the backends in one place? There's no set of flags that will let us show everything at once. Obviously this doesn't apply to the public-facing docs, which don't include backend info. |
|
Seems like yet another argument for getting rid of There's also a |
|
I took a look at replacing the It didn't work out as well as I hoped. Both of them pull in an optional dependency when activated. While that can be worked around with something like... [target.'cfg(dalek_backend = "fiat-crypto")'.dependencies]
fiat-crypto = "0.1.6"
So while we could do this, it would involve either |
|
My vote is to call this issue done for now and postpone switching to a |
|
Given the respective backends actually map to optional crate dependencies, I think features might actually be the best way of modeling them. However, perhaps we can make things more additive/feature-friendly by choosing a "winner" between It's a tough call though because they really do have diametrical opposite properties: one says "I want performance" and the other says "I want formally verified code, even at the cost of performance" (I haven't measured lately but last I checked I guess if we pick one it should probably be Edit: opened #437 to track support for activating both backend features at once. |
|
You can make |
|
Sounds like you're suggesting something like this? (my comment earlier)
I don't think it makes sense to do something that requires combinations of |
Features shouldn't be the opt-in knob to make use of unstable features, IMO, even when it feels weird. The only reason to use a feature here is that features are the only way to enable optional dependencies. |
|
#455 switches to |
|
Reopening this as the situation regressed somewhat in #523, which introduced a number of new crate features while removing Namely, it adds:
So we're back to a mixture of I'm not sure what the purpose of exposing such fine-grained control of the available backends is. @koute can you explain the rationale/use cases for offering such fine-grained control? The observation of this issue was curve25519-dalek previously had too many knobs which was leading to a lot of incidental complexity, and I feel like we're back to that again. Why would we want to enable the AVX-512 backend, but not the AVX2 backend, for example? Does it really make sense to have a crate feature associated with each backend, especially when the backends are platform specific? There are plans to add a NEON backend (#457) and potentially a GPU backend (#506) that need to be considered. Crate features can't be controlled by the toplevel binary, so when using My personal preference would be to continue leaning on |
I think we should keep in the course of not having features -given majority would probably not require them - please correct if / when wrong here with this assumption. Having too many options will cause a lot more complexity with education / documentation / auditing / testing / validating etc. and I think this would benefit being discussed together with all the cfg options including To keep it simple a plain "override" for This would also leave This would keep our education consistent - which we have already spent a decent amount of energy creating - and we can point to a single resource of setting "overrides" without potentially undocumented / easy to miss deviations. Keeping in mind that most people simply don't need overrides and it should always be controlled at binary level as the consensus here was - or is there a case where we would need education that would outweight the approach here ? Would be interested to hear. I regularly see crates across dependency trees where some crate in the middle didn't set features correctly and trying to upkeep featuresets consistent - when these are really configuration settings. I think trying to upkeep featuresets like this is a lost cause in general sense in my opinion where people don't have sole control of all the crates in a dependency tree. We already saw how broken 3.0.0 was by attempting to use the backend featuresets where even in-dalek ecosystem the feature flags were not relayed correctly in some cases or omitted together - main beneficiary would be the use-cases where one controls the whole dependency tree but I don't see a reason why they cannot use And for which we were going to put some effort for 5.0.0 release to more automatically set these as well known defaults:
From release PoV for 4.0.0 we should keep these plain overrides which will help testing and ensuring the implementation is correct given there is limited bandwidth to get changes for 4.0.0 and leave rest of the trickery for later release/s whether breaking or not - we should be less afraid of breaking major versions so we could potentially finally release 4.0.0 that people can anchor on. |
Well, I think it just makes sense to have those as discreet features that can be enabled or disabled, especially since those aren't supposed to change anything from a functional point of view. As a precedent the I suppose the primary use case would be testing? You're right though that it could be confusing to the end users as to which backend(s) is/are enabled, and that having a mix of This is purely a personal opinion, but honestly I would probably just do something like this:
So then we would achieve the following:
There are some projects (like the one I work on) which are huge frameworks where we control 98% of the dependency tree, but we don't have the control over the end user binary and how it's built. So we're the ones pulling |
|
I think we need more of the Why here rather than How. I don't think testing use-case justifies feature flags considering testing should be done at library level here in any case. It is not the curve25519-dalek downstream library's responsibility to test curve25519_dalek configuration knobs. If the downstream library from curve25519-dalek really wants to test the configuration knobs it can have a reference binary for that What kind of testing would be required at mid-layer consumer library level that needs these features that cannot be done here ? Is the only use-case to provide "white labeling" option for the mid-level library wants hide these overrides as point of abstraction ? Then we should be asking why does the mid-level library need control for this - I don't think there is use-case for giving control away to make it worthwhile effort to add further complexity just in case basis. fwiw - If we look at those esoteric Most people if they do end up knowing them in first place neither change these or never end up relaying the featureflags across the dependency chain. This literally happened with dalek crates earlier where it was not possible to use backends because these were not relayed correctly and integration testing to test the feature-chaining was hit and miss if it worked to begin with (it didn't 😆) Especially if these are at library in the middle of a long dependency chain where the whole regex affair gets buried deep down as implementation detail where everyone tends to forget these knobs going to category "never needed".
True - Most if not all users should not worry about these and I'm beginning to think we should not have any knobs for backend override at all that can be managed via dependency chain. Let's keep in mind that backend override (not a selection) is not the only knob we have - we have another well defined knob with clear justified use-cases for the If we do end up supporting this then we can do this at build script where the feature flag triggers appropriate It would be still great idea nonetheless to understand the actual use-case/s why somebody would be using these features so we can document and test them in the basis we do similarly with the bits override. Only remote thing I could think about as use-case to disable e.g. AVX512 where some single CPU model / batch would have some sort of esoteric bug that leads to overheating or some other unfortunate effect - but I don't think this should be the responsibility of mid-layer library in the middle to mitigate but the one who controls the binary who would be affected by this by using certain CPU model that is affected by some CPU related bug - or curve25519-dalek's which has the CPU detect in first place to address this strongly hypothetical case.
Why does a library in the middle need control of these configuration flags ? Could we please elaborate use-case/s for this ? |
This was exactly where we were before with The documentation for crate features has a lot of bad things to say about this approach as well: https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features "This should be avoided if at all possible. [...] Instead of using mutually exclusive features, consider some other options"
Aren't you breaking your own rule there? Are you saying that "libraries" shouldn't provide any What configuration do you want your framework to set and why is it overriding the defaults? Can you give a concrete example of where This is where it's nice that instead of documenting that as best practice (but still leaving the door open for clashing dependencies to break the build if developers overlook the fine print), |
In a strictly technical sense they're both libraries, but normal libraries don't pull in 1000 transitive dependencies (many of those are our own crates, but still) when you include them in a
But that's not exactly what I'm proposing here! (: What v3.x had was that the backend feature was set as a default feature flag. This means that if I do What I'm proposing here is to not set a backend in the If the intermediate libraries won't touch the backend selection features in any way then there won't be any conflicts. And if we don't break their compilation when Sure, there could still be libraries that would add passthrough feature-flags to select the backend and there wouldn't be much we could do about it, but that wouldn't be the default, and we could write in the docs "hey, don't do that, and if you do you're on your own!" or something along these lines.
Indeed. But if there's no alternative then personally I prefer using it to (Although here arguably there could be an alternative - allow backend selection at compile time through generics. It'd require some refactoring and increase compile times for end users though.)
Currently we wouldn't need to set those, but if we did we really can't use This is because, again, we're not really a normal library. Normal libraries are not composed of 250+ crates and pull in 1000+ dependencies. We're essentially a binary masquerading as a library.
I wholly agree that most if not all users should not care about the backend and not touch it at all, which is why, yes, what we had in 3.x was not great because they were forced to care. |
|
So to be clear, you want to reserve the right to potentially override Meanwhile going a 100% features route again would reintroduce a way for intermediate dependencies to break the build for their downstream users. Yes, we can document how to avoid that and hope people read the documentation, or alternatively we can prevent it from happening entirely via a design where the failure conditions are inexpressible. |
Not necessarily reserve; you asked me why we wouldn't be able to use a It's just I personally think that the worse user experience of having to use a So, to summarize, personally I'd just 1) make it implicitly select a default backend if no features are provided, 2) name all of the backend features And yes, you're right, that would reintroduce a way for intermediate dependencies to break the build for their downstream users. It is a tradeoff. Anyway, if I can't convince you then I don't intend to press the matter further. (: You're right that we technically don't need this now, so fair point! |
|
I do wish there was some way to solve the discoverability problem; I have to look individually at the docs for every indirect dependency I have to know if it responds to custom cfg flags or environment variables. Cargo features are only a little better here in that they could be collected mechanically, but I don't think they are. I don't have any strong ideas, though, only things like "eprint a warning in build.rs if the user hasn't explicitly chosen a backend or 'auto'", which I think would get old fast for every intermediate library. |
We use That said, I think >99% of the time there will be no need to set a There are only three situations presently where you'd hypothetically set one:
Hypothetically we might add "use a GPU backend" but that's the only foreseeable other one. All of these things seem like concerns for the toplevel binary. |
|
PR's up based on consensus to close this out |
curve25519-dalekcurrently defines the following backends, modeled as crate features:u32_backendu64_backendfiat_u32_backendfiat_u64_backendsimd_backendavx2_backendFirst,
avx2_backendcan be removed: it's a deprecated legacy alias forsimd_backend.Second, I think
u32_backendandu64_backendcould be selected automatically by gating on e.g.cfg(target_pointer_width = "64"). If this selection were automatic, I think these features could be removed completely, withu32_backend/u64_backendselected and used by default unless another backend has been explicitly enabled.Likewise,
fiat_u64_backendandfiat_u32_backendcan be collapsed down tofiat_backendusing similartarget_pointer_width-based gating.If all of the above were done, the list above could be simplified down to just
fiat_backendandsimd_backend.The text was updated successfully, but these errors were encountered: