Skip to content

elide pass-by-ref in root functions#21

Merged
bors[bot] merged 1 commit intorust-num:masterfrom
strake:sqrt
Mar 29, 2019
Merged

elide pass-by-ref in root functions#21
bors[bot] merged 1 commit intorust-num:masterfrom
strake:sqrt

Conversation

@strake
Copy link
Copy Markdown
Contributor

@strake strake commented Mar 28, 2019

No description provided.

@cuviper
Copy link
Copy Markdown
Member

cuviper commented Mar 29, 2019

Presumably this is for performance -- do you have a benchmark comparison? Can you get the same effect by just binding let a = *self; at the start, rather than nesting another function?

@strake
Copy link
Copy Markdown
Contributor Author

strake commented Mar 29, 2019

Presumably this is for performance -- do you have a benchmark comparison?

here — it reliably cuts about 1/5 off the time for {i128, u128}::sqrt_rand in particular, and mostly speeds up the others a little also (on my machine).

Can you get the same effect by just binding let a = *self; at the start, rather than nesting another function?

No, the compiler already lifts the load to the start of the function — this PR elides the load altogether. The problem is the trait methods take self by reference.

@cuviper
Copy link
Copy Markdown
Member

cuviper commented Mar 29, 2019

Ah, I did some profiling, and now I see it's a difference of inlining. The current code doesn't inline at all, but with your change it's inlining the outer &self method and calling sqrt::go by value. That's what lets it avoid the reference and load altogether.

Thanks!

bors r+

bors Bot added a commit that referenced this pull request Mar 29, 2019
21: elide pass-by-ref in root functions r=cuviper a=strake



Co-authored-by: M Farkas-Dyck <strake888@gmail.com>
@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Mar 29, 2019

Build succeeded

@bors bors Bot merged commit 956315c into rust-num:master Mar 29, 2019
@strake strake deleted the sqrt branch March 30, 2019 05:50
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.

2 participants