1374848 - stylo: SmallVec::into_iter does unnecessary memmovs

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

See https://github.com/rust-lang/rust/issues/42763

The best workaround is probably switching all the callsites to drain. Very annoying though. Anyone have any clever ideas?

Comment 1

[Simon Sapin (:SimonSapin)]

We can start by adding #[inline] on the relevant into_into method and see if that helps. Beyond that, it’s really in the realm of compiler implementation.

By the way, Rust 1.20 is currently in Nightly. If I’m reading calendars right it goes to Beta on 2017-07-20, and reaches Stable on 2017-08-31. Firefox 57 goes to Beta on 2017-09-26. So if want to rely in 57 on a new rustc optimization, we have ~one month to land it and have it safely ride the trains of both projects. Unless we’re willing to update a compiler during the 57 beta cycle; 1.21 is planned for 2017-10-12.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #1)
> We can start by adding #[inline] on the relevant into_into method and see if
> that helps.

Hm why would it help? It already gets inlined in https://gist.github.com/bholley/69dcf3654dfde0ebd8bb8bd8a7c2386d , and the codegen is the same. The problem is that the inliner isn't doing its job.

> Beyond that, it’s really in the realm of compiler implementation.
> 
> By the way, Rust 1.20 is currently in Nightly. If I’m reading calendars
> right it goes to Beta on 2017-07-20, and reaches Stable on 2017-08-31.
> Firefox 57 goes to Beta on 2017-09-26. So if want to rely in 57 on a new
> rustc optimization, we have ~one month to land it and have it safely ride
> the trains of both projects. Unless we’re willing to update a compiler
> during the 57 beta cycle; 1.21 is planned for 2017-10-12.

I'm not suggesting any rust compiler fixes at this point. I'm suggesting replacing all our usage of SmallVec::into_iter with SmallVec::drain (which I think emilio has started doing already).

Comment 3

[Simon Sapin (:SimonSapin)]

> Hm why would it help? It already gets inlined in https://gist.github.com/bholley/69dcf3654dfde0ebd8bb8bd8a7c2386d , and the codegen is the same. The problem is that the inliner isn't doing its job.

There is no explicit #[inline] in

    impl<A: Array> IntoIterator for SmallVec<A> {
        type IntoIter = IntoIter<A>;
        type Item = A::Item;
        fn into_iter(mut self) -> Self::IntoIter {


so I assumed it wouldn’t get inlined, but now I remembered that cross-crate inlining is still possible since the type is generic.


> I'm not suggesting any rust compiler fixes at this point. I'm suggesting replacing all our usage of SmallVec::into_iter with SmallVec::drain (which I think emilio has started doing already).

Ok, sounds good.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #3)
> so I assumed it wouldn’t get inlined, but now I remembered that cross-crate
> inlining is still possible since the type is generic.

More to the point, there's only one crate involved here - I just copied the source of SmallVec inline in my gist so that I could stick it all in Playground [1], and the asm we get out of playground has the memmov.

[1] If there's some better way to import crates.io code on playground, I would be very interested to hear it.

Comment 5

[Simon Sapin (:SimonSapin)]

In general, having something (SmallVec and its IntoInter impl) in one crate and code using in another crate will not necessarily compile to the same machine code as having everything in the same crate, especially without LTO. Though maybe in this case it doesn’t make a difference since generics are monomorphized where they’re used.

> If there's some better way to import crates.io code on playground

As far as I know there is not. I’ve been using https://github.com/DanielKeep/cargo-script locally for quick single-file experiments.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Simon Sapin (:SimonSapin) from comment #5)
> In general, having something (SmallVec and its IntoInter impl) in one crate
> and code using in another crate will not necessarily compile to the same
> machine code as having everything in the same crate, especially without LTO.
> Though maybe in this case it doesn’t make a difference since generics are
> monomorphized where they’re used.

Oh sure - I'm just saying that it's unlikely that the compiler would figure out how to elide the move in the cross-crate but not the same-crate case (the other way around would be more likely).

Comment 7

[Bobby Holley (:bholley)]

Pushed [1] to deal with the last remaining callers of SmallVec::into_iter in the style system. This is now a non-issue for stylo.

[1] https://github.com/servo/servo/pull/17952