Use reverse_bits intrinsic when supported #16

thomwiggers · 2018-05-28T14:21:37Z

This adds a new feature that allows to conditionally use the new
reverse_bits feature in Rust.

ltratt · 2018-05-28T16:37:59Z

Does this provide a noticeable speed-up?

ltratt · 2018-05-29T06:58:28Z

Thinking about this a bit more: assuming this does give a useful speed-up (which I imagine it does, but we need to see numbers to be sure), wouldn't it be easier to use this automatically if nightly is used to build vob? Put another way: if it works well, I can't imagine anyone not wanting to use this feature. This would also make it completely transparent to the user, and mean that we don't have any future headaches removing a Cargo feature which some people have come to rely on.

thomwiggers · 2018-05-29T09:37:37Z

Does this provide a noticeable speed-up?
CI is running benchmarks when running builds on nightly; so you should be able to see the numbers there.

There is a slight speed-up on my system, but it should be more noticeable on e.g. ARMv8 where there is CPU support.

wouldn't it be easier to use this automatically if nightly is used to build vob? Put another way: if it works well, I can't imagine anyone not wanting to use this feature. This would also make it completely transparent to the user, and mean that we don't have any future headaches removing a Cargo feature which some people have come to rely on.

Unfortunately, I don't know of a way to detect if a feature is available in 'normal' rust. There's ways to set a feature flag using a build.rs, but that seems a bit excessive. Also, once it stabilizes, you kind of still want to build with the feature flag turned on to opt in to the new behaviour - but detecting nightly would not be sufficient to do that anymore.

See also https://internals.rust-lang.org/t/setting-cfg-nightly-on-nightly-by-default/1893

ltratt · 2018-05-29T13:00:59Z

What sort of speed-up did you see? And, assuming it's an improvement, why would we want to make this an opt-in feature? It feels like it should be something the user doesn't know about: it just makes their code go faster.

thomwiggers · 2018-05-29T15:23:25Z

4k ns to 3k or something.

It's not that I wouldn't want to, it's just that I can't figure out how to do it (without 50 extra lines of code).

ltratt · 2018-06-01T13:38:46Z

I must admit, for the complexity involved, I'm still wondering if we should just wait for this to stabilise. Admittedly, I'm not sure when it will stabilise.

ltratt · 2018-06-25T07:09:31Z

In the interests of not letting things languish without a decision forever I think we should say that we won't merge this unless it can be detected automatically (with build.sh or some other trick). And if we don't want to do such a trick, it's probably better to close the PR. Sound like a plan?

thomwiggers · 2018-06-25T07:15:03Z

That makes sense. Let's mark it WIP for now.

ltratt · 2018-06-25T07:29:51Z

OK, I'll close this, but of course you can reopen it if/when automatic detection works.

thomwiggers · 2018-06-25T08:44:58Z

Ok, I had some procrastination from writing on my master's thesis to do, so I implemented detection of nightly on top of the feature flags that are available.

There's now a nightly cfg flag that is automatically set on nightly. Its use is to enable the #![feature] tags. Those will only ever work on nightly, anyway.

The nightly flag is separate from the reverse_bits cfg flag. That flag will be set if nightly is detected and the no_reverse_bits feature is not enabled, or if the reverse_bits feature is enabled. build.rs contains a sanity check that both aren't enabled at the same time.

The reverse_bits feature will not switch on the #[feature] flag, to allow switching it on in stable. Of course, that won't work for the time being, but it will allow people that use Vob to opt-in to the new behaviour whenever rust-lang/rust#48763 is stabilized, without a Vob update. (Though we should obviously still keep track of that issue)

By the way, I recommend switching on the cron settings of Travis to check nightly builds daily.

The number of builds is something we can talk about 😛.

ltratt · 2018-06-25T09:37:01Z

build.rs

+    );
+
+    // Set features depending on channel
+    match version_meta().unwrap().channel {


Would this better be if let Channel::Nightly = version_meta().unwrap().channel { ... } else { ... }?

done, especially relevant since we no longer handle the feature settings.

ltratt · 2018-06-25T09:38:51Z

Cargo.toml

@@ -20,3 +23,4 @@ rand = "0.5"
 [features]
 unsafe_internals = []
 reverse_bits = []


I don't think we need reverse_bits and no_reverse_bits. Personally I'd delete both, and just silently use reverse bits; but if we really, really want an option I think no_reverse_bits is enough.

deleted the features.

ltratt · 2018-06-25T09:39:26Z

Are we not able to query rustc directly for whether the reverse_bits features is available?

thomwiggers · 2018-06-25T09:44:04Z

Are we not able to query rustc directly for whether the reverse_bits features is available?

AFAIK, no. You could perhaps trail-compile something, but that seems way too much. That's why I included the reverse_bits feature.

ltratt · 2018-06-25T09:45:09Z

@vext01 Do you know if you can query rustc for a feature directly?

@thomwiggers If not, I agree your approach is reasonable.

vext01 · 2018-06-25T09:48:13Z

You mean like:

#[cfg(feature = "foo")]

?

https://doc.rust-lang.org/1.26.0/book/first-edition/conditional-compilation.html

ltratt · 2018-06-25T09:50:35Z

Not exactly. We'd like to detect when this feature https://doc.rust-lang.org/std/primitive.isize.html#method.reverse_bits is available or not (at the moment we assume "if we're on nightly, it's available", which might be a bit fragile).

vext01 · 2018-06-25T09:54:58Z

What do you want to do if it's not available?

ltratt · 2018-06-25T09:55:44Z

If it's not available, we have a fallback (slower) route that doesn't involve using the feature.

vext01 · 2018-06-25T09:58:14Z

OK, so it would look like this I think:

#[cfg(feature = "reverse_bits")] {
    // fast code
}
#[cfg(not(feature = "reverse_bits"))] {
    // slow code
}

thomwiggers · 2018-06-25T10:02:11Z

That only works for Cargo.toml-defined features.

vext01 · 2018-06-25T10:03:28Z

Ugh. That being the case, no idea then. Sorry.

vext01 · 2018-06-25T10:04:01Z

(I reckon that is a bug in rustc, why should compiler features be special?)

vext01 · 2018-06-25T10:04:41Z

The only other thing you can do is make a feature check in a build.rs, but that's horrible too.

ltratt · 2018-06-25T10:07:30Z

OK, then I think Thom's suggestion is as good as it gets. We can live with it. There are still a couple of other comments for @thomwiggers though :)

ltratt · 2018-06-25T10:08:24Z

.travis.yml

@@ -1,13 +1,27 @@
 language: rust

+rust:
+  - stable
+  - beta


I must admit, I'm not very sure it's useful for us to test on beta. I know it doesn't hurt, but I feel a bit bad for using electricity for something that's probably never going to show an error.

Seems like a good way to eliminate a lot of builds as well

ltratt · 2018-06-28T08:03:23Z

Cargo.toml

@@ -19,3 +22,5 @@ rand = "0.5"

 [features]
 unsafe_internals = []
+reverse_bits = []


Are these two lines supposed to be deleted?

hmm somehow I missed them

I thought you might be testing me :p

ltratt · 2018-06-28T08:35:31Z

OK, I think this is ready to be squashed.

thomwiggers · 2018-06-29T08:46:58Z

It's been squashed by the way

thomwiggers · 2018-06-29T08:47:38Z

build.rs

+
+fn main() {
+    assert!(
+        !(env::var("CARGO_FEATURE_REVERSE_BITS").is_ok()


oh wait, this needs to go.

Detect nightly and auto-enable reverse_bits

ltratt · 2018-06-29T13:22:13Z

Thanks!

ltratt self-assigned this May 29, 2018

thomwiggers force-pushed the fast_reverse branch 2 times, most recently from 6e6772b to 0ff2269 Compare May 29, 2018 09:23

thomwiggers force-pushed the fast_reverse branch 2 times, most recently from 1d653ae to f615b80 Compare May 30, 2018 11:36

thomwiggers force-pushed the fast_reverse branch 3 times, most recently from 88d8c8f to 7c1913b Compare June 5, 2018 06:28

thomwiggers added the enhancement New feature or request label Jun 20, 2018

thomwiggers force-pushed the fast_reverse branch from 7c1913b to e94b1bd Compare June 22, 2018 07:27

thomwiggers changed the title ~~Use reverse_bits intrinsic based on feature~~ WIP: Use reverse_bits intrinsic based on feature Jun 25, 2018

thomwiggers changed the title ~~WIP: Use reverse_bits intrinsic based on feature~~ WIP: Use reverse_bits intrinsic when supported Jun 25, 2018

ltratt closed this Jun 25, 2018

thomwiggers changed the title ~~WIP: Use reverse_bits intrinsic when supported~~ Use reverse_bits intrinsic when supported Jun 25, 2018

thomwiggers reopened this Jun 25, 2018

ltratt reviewed Jun 25, 2018

View reviewed changes

ltratt reviewed Jun 28, 2018

View reviewed changes

thomwiggers force-pushed the fast_reverse branch from b6ad9f8 to fe810e8 Compare June 28, 2018 08:49

thomwiggers commented Jun 29, 2018

View reviewed changes

thomwiggers force-pushed the fast_reverse branch from fe810e8 to 7844496 Compare June 29, 2018 08:48

Use reverse_bits intrinsic when running on nightly

ef0d020

Detect nightly and auto-enable reverse_bits

thomwiggers force-pushed the fast_reverse branch from 7844496 to ef0d020 Compare June 29, 2018 08:49

ltratt approved these changes Jun 29, 2018

View reviewed changes

ltratt merged commit bbef6a0 into softdevteam:master Jun 29, 2018

Use reverse_bits intrinsic when supported #16

Use reverse_bits intrinsic when supported #16

Conversation

thomwiggers commented May 28, 2018

ltratt commented May 28, 2018

ltratt commented May 29, 2018

thomwiggers commented May 29, 2018

ltratt commented May 29, 2018

thomwiggers commented May 29, 2018 • edited Loading

ltratt commented Jun 1, 2018

ltratt commented Jun 25, 2018

thomwiggers commented Jun 25, 2018

ltratt commented Jun 25, 2018

thomwiggers commented Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

thomwiggers Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltratt commented Jun 25, 2018

thomwiggers commented Jun 25, 2018

ltratt commented Jun 25, 2018

vext01 commented Jun 25, 2018

ltratt commented Jun 25, 2018

vext01 commented Jun 25, 2018

ltratt commented Jun 25, 2018

vext01 commented Jun 25, 2018

thomwiggers commented Jun 25, 2018

vext01 commented Jun 25, 2018

vext01 commented Jun 25, 2018

vext01 commented Jun 25, 2018

ltratt commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltratt commented Jun 28, 2018

thomwiggers commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltratt commented Jun 29, 2018

thomwiggers commented May 29, 2018 •

edited

Loading

thomwiggers commented Jun 25, 2018 •

edited

Loading

thomwiggers Jun 27, 2018 •

edited

Loading