fitzgen / inlinable_string

An owned, grow-able UTF-8 string that stores small strings inline and avoids heap-allocation.
http://fitzgen.github.io/inlinable_string/inlinable_string/index.html
Other
69 stars 11 forks source link

Reuse `AsRef` and `AsMut` as possible to reduce unsafe #25

Open lo48576 opened 4 years ago

lo48576 commented 4 years ago

Some trait implementations share the same codes which contain unsafe block, but they can be shared to reduce duplicates.

ArtemGr commented 4 years ago

Hi, Yoshioka! I hope everything's super nice where you are!

Thanks for the patch!

I've noticed that we don't have #[inline] on AsRef<str> and AsMut<str>. As such there might be a semantic difference in that previously we were asking of Rust to inline the entire Index code path and now we're asking it to index only a part of the code path.

I understand that the actual difference might be non-existent or hard to measure due to LLVM optimizations, but I'd say that with this patch we depend on the LLVM optimizations more than before.

Also I wonder if #[inline] works even before LLVM, maybe in MIR, resulting in different optimization opportunities.

Also the extra indirection, going through two traits and through a pointer, means there is more chances for optimizations not to work for some reason (compilers are complex and evolving beasts).

Also I suspect that the non-optimized debug code paths will now be longer, which might seriously impact Index-based tight loops for someone.

Do you think if maybe some of these concerns are valid?

lo48576 commented 4 years ago

TL; DR:

#[inline] for AsRef<str> and AsMut<str>

I'm surprised that As{Ref,Mut}<str> don't have #[inline] attributes. Their function body are (were) completely same as Deref{,Mut} and Index{,Mut}<RangeFull>, so I think there are no reasons of AsRef and AsMut not having #[inline] attributes.

Optimization difference

I've heard that excessive use of inline attributes sometimes prevents compilers from generating better codes. Also, I've heard that inline attributes are just hints for compilers, and functions are sometimes not inlined even if it is told to be inlined. (I'm afraid I can't tell details because I don't understand it well and I'm not pro of compilers.)

So, I don't think we can control compiler optimization very well with #[inline] attributes. There might be differences in generated codes between compiler versions and function call nest levels, but Rust would be too high-level to ensure machine codes are optimal...

Extra indirection

I confirmed that extra indirection will cause extra call instruction (https://rust.godbolt.org/z/8XK3b_). However, I think this is acceptable overhead for debug build. Such single-level indirection happens everywhere as usual.

For exmaple, "hello" == "hello" is desugared to <&str as PartialEq<&str>>::eq() and it calls <str as PartialEq<str>>::eq() internally, but I've never cared about such indirection. Anoter example is for elem in &vec { ... }: it is desugared to for elem in <&Vec<_> as IntoIterator>::into_iter(&vec). IntoIterator::into_iter(&vec) calls vec.iter() internally, so theoretically for elem in vec.iter() is faster in debug build, but we would prefer &vec even if it is inside another loop.

I personally think it is better to make code easier to maintain, rather than to allow duplicate codes to make it faster.

ArtemGr commented 4 years ago

I personally like simple clean code, even if it makes optimization non-obvious or cause small overhead. I think such costs are acceptable.

I'm of an opinion that in every project we have three implicit priorities: development speed, code quality and performance. These priorities might be very different for different kinds of code.

[inline] for AsRef and AsMut

We can add inline there of course.

So, I don't think we can control compiler optimization very well with #[inline] attributes.

Here's a recent example of using explicit inlining in Rust compiler: https://github.com/rust-lang/rust/pull/69256.

I confirmed that extra indirection will cause extra call instruction (https://rust.godbolt.org/z/8XK3b_).

Good to know, thanks!