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.

Sign up for GitHub

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

Jump to bottom

Improve pow speed for expensive types by multiplying references not cloned values (reopen) #153

Closed
wants to merge 1 commit into from
Closed

Improve pow speed for expensive types by multiplying references not cloned values (reopen) #153

wants to merge 1 commit into from

Conversation

dten
Copy link
Contributor

@dten dten commented Jan 7, 2016

Resubmission of #152
Couldn't reopen because I broke the branch

Speed up on pow_bench

   test pow_bench         ... bench:   9,579,445 ns/iter (+/- 560,439)
-> test pow_bench         ... bench:   6,820,137 ns/iter (+/- 760,415)

@cuviper
Copy link
Member

cuviper commented Jan 7, 2016

Great. As noted in #152, we'll hold this until we're ready to make breaking changes.

@cuviper cuviper mentioned this pull request Jan 7, 2016
Closed
6 tasks
@dten dten force-pushed the pr-powmulref branch 2 times, most recently from 3924a8e to 912191a Compare January 9, 2016 14:09
@est31 est31 mentioned this pull request Feb 3, 2016
Merged
homu added a commit that referenced this pull request Feb 6, 2016
@homu
Auto merge of #162 - est31:add_checked_pow, r=cuviper
d1adf45 
Add checked_pow function

Implements a `checked_pow` function which does the same as `pow`, just with overflow checks.

And, similar to #152 and #153, the function uses references instead of cloning.

Adds a little macro to spare code repetition. Its scoped to the function so nothing gets polluted.
@vks
Copy link
Contributor

vks commented Mar 9, 2016

I think this could be even more efficient by using x = x * &y instead of x = &x * &y.

@dten
Copy link
Contributor Author

dten commented Mar 20, 2016

@vks i suspect you're right 👍

@homu
Copy link
Contributor

homu commented May 13, 2016

☔ The latest upstream changes (presumably #192) made this pull request unmergeable. Please resolve the merge conflicts.

@hauleth hauleth modified the milestone: v0.2.0 Sep 18, 2016
@cuviper cuviper mentioned this pull request Feb 10, 2017
Closed
@vks vks mentioned this pull request Mar 17, 2017
Closed
@cuviper
Copy link
Member

cuviper commented Sep 20, 2017

Do you want to move this PR to the next branch? That's where I'm (slowly) making breaking changes, although I'm not sure when that will get around to a release.

@dten
Copy link
Contributor Author

dten commented Sep 21, 2017

That sounds the best place for it yes please

@cuviper cuviper changed the base branch from master to next September 21, 2017 19:07
@cuviper
Copy link
Member

cuviper commented Sep 21, 2017

Great, let's go!

bors r+

bors bot added a commit that referenced this pull request Sep 21, 2017
@bors
Merge #153
de0e917 
153: Improve pow speed for expensive types by multiplying references not cloned values (reopen) r=cuviper a=dten

Resubmission of #152
Couldn't reopen because I broke the branch

Speed up on pow_bench

```
   test pow_bench         ... bench:   9,579,445 ns/iter (+/- 560,439)
-> test pow_bench         ... bench:   6,820,137 ns/iter (+/- 760,415)
```
@bors
Copy link
Contributor

bors bot commented Sep 21, 2017

Build failed

@cuviper
Copy link
Member

cuviper commented Sep 21, 2017

Hmm, in next we have a default implementation of Float::powi using this pow, but Float doesn't have reference operators. But I suppose we could expand those requirements...

@cuviper
Copy link
Member

cuviper commented Sep 21, 2017

Oh, but indirect trait requirements don't propagate well. (rust-lang/rust#20671)

I guess we can just duplicate the powi implementation by value for this case. It's not that much code.

@cuviper cuviper mentioned this pull request Sep 21, 2017
Merged
bors bot added a commit that referenced this pull request Sep 21, 2017
@bors
Merge #334
9a4729f 
334: Use `Mul` by reference in `pow` r=cuviper a=cuviper

Resubmission of #153, since it wasn't open to maintainer edits.  I added a distinct `Float::powi` fallback since that doesn't have `Mul` by reference.

cc @dten
@cuviper
Copy link
Member

cuviper commented Sep 21, 2017

Merged in #334, thanks!

@cuviper cuviper closed this Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
breaking change
Projects
None yet
Milestone
v0.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@dten @cuviper @vks @homu @hauleth