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

[Question] Is return-value optimization restrained? #64

Closed
y-ich opened this issue Aug 2, 2017 · 7 comments
Closed

[Question] Is return-value optimization restrained? #64

y-ich opened this issue Aug 2, 2017 · 7 comments
Labels

Comments

@y-ich
Copy link

y-ich commented Aug 2, 2017

Hi.

I have a fuction that returns large ArrayVec instance.
I guess that there are no return-value optimizations in release build and the instance is copied into stack every time when it returns.
(I am not quite sure since assemble code is complicated for me.)

Is my understanding right?
And if so, are there any ways for return-value optimizations?

Thanks.

@bluss
Copy link
Owner

bluss commented Aug 2, 2017

Hi, I haven't examined this in detail, so if you have examples and their results it's worthwhile. Rust knows how to do some RVO but not always; I think there are also still improvements that Rust or llvm could do w.r.t to this.

  1. Does it change if you enable the use_union feature (nightly)? This equivalent something we can do by default in stable Rust after Rust 1.20 is released.

@bluss bluss added the question label Aug 2, 2017
@y-ich
Copy link
Author

y-ich commented Aug 2, 2017

Thank you for your comment.

There are no changes by use_union.

Here is a gist that I did.
https://gist.github.com/y-ich/ae52392af39e46e5af321106b379b818

And here is my observation.

  • Returning an array results in return-value optimization but it needs initialization.
  • I made a simplified version of arrayvec, which has no impl of Drop, it is faster then arrayvec, but still it cause a copy for return.

@bluss
Copy link
Owner

bluss commented Aug 2, 2017

I don't doubt that there are problems, but black_box could inhibit RVO, we need to look at tests without it, even without debug = true.

Brief look using cargo rustc --release -- --emit=asm

It says that only make_array, make_array_in_var, make_array_literal have RVO, which is not so good.

extern crate arrayvec;
use arrayvec::ArrayVec;

const N: usize = 128;

pub fn make_array() -> ArrayVec<[u8; N]> {
    ArrayVec::new()
}

pub fn make_array_in_var() -> ArrayVec<[u8; N]> {
    let v = ArrayVec::new();
    v
}

pub fn make_array_with_1() -> ArrayVec<[u8; N]> {
    let mut v = ArrayVec::new();
    v.push(0);
    v
}

pub fn make_array_collect() -> ArrayVec<[u8; N]> {
    [1, 2, 3].iter().cloned().collect()
}

pub fn make_array_literal() -> ArrayVec<[u8; N]> {
    ArrayVec::from([0; N])
}

@y-ich
Copy link
Author

y-ich commented Aug 2, 2017

Thank you for your analysis!
Benchmarks of optimizations always trouble me.

It seems that assignment and operations disturb the optimization...

@bluss
Copy link
Owner

bluss commented Aug 2, 2017

Examples in plain rust arrays. one has RVO, two has not. There are some open issues about "NRVO" and move propagation / destination propagation over in the Rust issues, but I'm unsure what exactly tracks this.

https://play.rust-lang.org/?gist=c7f63d4dd522a5bbcc7be97f43c70743&version=nightly

const N: usize = 128;

pub fn one() -> [u8; N] {
    let a = [0; N];
    a
}

pub fn two() -> [u8; N] {
    let mut a = [0; N];
    a[0] = 1;
    a
}

@bluss
Copy link
Owner

bluss commented Sep 11, 2017

rustc has changed. There is RVO for both one and two, in nightly right now.

@bluss
Copy link
Owner

bluss commented Sep 11, 2017

arrayvec test cases seem unchanged.

Right now make_array has this output (this seems ok, but for future reference).

	.type	_ZN3rvo10make_array17h5b72164c37215d2eE,@function
_ZN3rvo10make_array17h5b72164c37215d2eE:
	.cfi_startproc
	movb	$0, (%rdi)
	movb	$0, 65(%rdi)
	movq	%rdi, %rax
	retq
.Lfunc_end0:
	.size	_ZN3rvo10make_array17h5b72164c37215d2eE, .Lfunc_end0-_ZN3rvo10make_array17h5b72164c37215d2eE
	.cfi_endproc

@bluss bluss closed this as completed Dec 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants