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 clone_from impls for collections #28602

Merged
merged 2 commits into from
Sep 24, 2015
Merged

Improve clone_from impls for collections #28602

merged 2 commits into from
Sep 24, 2015

Conversation

apasel422
Copy link
Contributor

r? @bluss

@dotdash
Copy link
Contributor

dotdash commented Sep 23, 2015

@bors r+

Nice!

@bors
Copy link
Contributor

bors commented Sep 23, 2015

📌 Commit 85be55b has been approved by dotdash

@bors
Copy link
Contributor

bors commented Sep 23, 2015

⌛ Testing commit 85be55b with merge 8de4886...

@bluss
Copy link
Member

bluss commented Sep 23, 2015

Did you push a new commit after bors tested the old one. What was the change?

bluss
bluss reviewed Sep 23, 2015
View reviewed changes
src/libcollections/vec.rs
for (place, thing) in self.iter_mut().zip(other) {
place.clone_from(thing)
{
let src = &other[..len];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This trick might warrant a comment.

@apasel422
Copy link
Contributor Author

@bluss Yeah, I pushed a new commit, so bors hasn't tested the new one yet. I removed an unnecessary call to cmp::min, since truncate guarantees that the destination's length is less than or equal to the source's.

I'll add a comment that explains the trick and re-push.

@bluss
Copy link
Member

bluss commented Sep 23, 2015

Great

@apasel422
Copy link
Contributor Author

Hmm, instead of using the trick, should I just use a more explicit (and less finicky) get_unchecked loop like @Veedrac did in the related issue?

@bluss
Copy link
Member

bluss commented Sep 23, 2015

It's almost about time we package up this "zip slices" pattern now in a function or iterator actually, since we have to use it in a lot of places.

I think your question is interesting:

  1. Write direct, do what I say, unsafe code (get_unchecked)
  2. Write a particular pattern that we know the optimizer understands

(2) can absolutely be called fragile. I still like (2) better since it's safe, it degrades well (still correct even if the optimizer's behavior changes).

@apasel422
Copy link
Contributor Author

It's actually already present in clone_from_slice, from what I can tell.

@bluss
Copy link
Member

bluss commented Sep 23, 2015

yes, for clone in particular it seems it is. We can reuse that here, then.

@apasel422
Copy link
Contributor Author

Updated.

@bluss
Copy link
Member

bluss commented Sep 23, 2015

Now it's beautiful

@bors r+

@bors
Copy link
Contributor

bors commented Sep 23, 2015

📌 Commit 2f1084a has been approved by bluss

@bors
Copy link
Contributor

bors commented Sep 24, 2015

⌛ Testing commit 2f1084a with merge 450f87f...

@bors
Copy link
Contributor

bors commented Sep 24, 2015

💔 Test failed - auto-mac-64-opt

@apasel422
Optimize Vec::clone_from
97f2a32 
Before:

test dst_bigger::src_100_dst_1000::clone      ... bench:  34 ns/iter (+/- 1)
test dst_bigger::src_100_dst_1000::clone_from ... bench:  75 ns/iter (+/- 3)

test dst_bigger::src_10_dst_100::clone        ... bench:  25 ns/iter (+/- 0)
test dst_bigger::src_10_dst_100::clone_from   ... bench:   9 ns/iter (+/- 1)

test eq::src_1000_dst_1000::clone             ... bench: 105 ns/iter (+/- 2)
test eq::src_1000_dst_1000::clone_from        ... bench: 593 ns/iter (+/- 21)

test eq::src_100_dst_100::clone               ... bench:  34 ns/iter (+/- 1)
test eq::src_100_dst_100::clone_from          ... bench:  75 ns/iter (+/- 1)

test src_bigger::src_1000_dst_100::clone      ... bench: 103 ns/iter (+/- 5)
test src_bigger::src_1000_dst_100::clone_from ... bench: 148 ns/iter (+/- 5)

test src_bigger::src_100_dst_10::clone        ... bench:  34 ns/iter (+/- 1)
test src_bigger::src_100_dst_10::clone_from   ... bench:  20 ns/iter (+/- 0)

After:

test dst_bigger::src_100_dst_1000::clone      ... bench:  34 ns/iter (+/- 2)
test dst_bigger::src_100_dst_1000::clone_from ... bench:  15 ns/iter (+/- 1)

test dst_bigger::src_10_dst_100::clone        ... bench:  26 ns/iter (+/- 1)
test dst_bigger::src_10_dst_100::clone_from   ... bench:   7 ns/iter (+/- 0)

test eq::src_1000_dst_1000::clone             ... bench: 103 ns/iter (+/- 1)
test eq::src_1000_dst_1000::clone_from        ... bench:  85 ns/iter (+/- 4)

test eq::src_100_dst_100::clone               ... bench:  34 ns/iter (+/- 2)
test eq::src_100_dst_100::clone_from          ... bench:  15 ns/iter (+/- 1)

test src_bigger::src_1000_dst_100::clone      ... bench: 103 ns/iter (+/- 4)
test src_bigger::src_1000_dst_100::clone_from ... bench:  90 ns/iter (+/- 2)

test src_bigger::src_100_dst_10::clone        ... bench:  34 ns/iter (+/- 2)
test src_bigger::src_100_dst_10::clone_from   ... bench:  20 ns/iter (+/- 0)

Closes #28601.
@apasel422
Copy link
Contributor Author

Fixed the test failure.

@bluss
Copy link
Member

bluss commented Sep 24, 2015

@bors r+

thanks!

@bors
Copy link
Contributor

bors commented Sep 24, 2015

📌 Commit 97f2a32 has been approved by bluss

bors added a commit that referenced this pull request Sep 24, 2015
@bors
Auto merge of #28602 - apasel422:clone_from, r=bluss
355bbfb 
r? @bluss
@bors
Copy link
Contributor

bors commented Sep 24, 2015

⌛ Testing commit 97f2a32 with merge 355bbfb...

@bors
Copy link
Contributor

bors commented Sep 24, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit 97f2a32 into rust-lang:master Sep 24, 2015
@bors bors mentioned this pull request Sep 24, 2015
Closed
@apasel422 apasel422 deleted the clone_from branch September 24, 2015 17:08
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bluss bluss

Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apasel422 @dotdash @bors @bluss