Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse AsRef and AsMut as possible to reduce unsafe #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lo48576
Copy link
Contributor

@lo48576 lo48576 commented Feb 20, 2020

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

Some trait implementations share the same codes which contain
`unsafe` block, but they can be shared to reduce duplicates.
@ArtemGr
Copy link
Collaborator

ArtemGr commented Feb 28, 2020

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
Copy link
Contributor Author

lo48576 commented Feb 29, 2020

TL; DR:

  • AsRef<str> and AsMut<str> can be #[inline]d.
  • I personally like simple clean code, even if it makes optimization non-obvious or cause small overhead.
    I think such costs are acceptable.

#[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
Copy link
Collaborator

ArtemGr commented Feb 29, 2020

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: rust-lang/rust#69256.

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

Good to know, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants