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

Avoid needless allocations in liveness_of_locals. #51869

Merged
merged 1 commit into from
Jul 1, 2018

Conversation

nnethercote
Copy link
Contributor

We don't need to replace the heap-allocated bitset, we can just
overwrite its contents.

This speeds up most NLL benchmarks, the best by 1.5%.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2018

/// Overwrite one `IdxSetBuf` with another of the same length.
pub fn overwrite(&mut self, other: &IdxSetBuf<T>) {
self.bits.clone_from_slice(&other.bits);
Copy link
Contributor

Choose a reason for hiding this comment

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

This method probably belongs on IdxSet rather than IdxSetBuf (note that IdxSetBuf derefs to IdxSet). The relationship between those two types is analogous to [] and Vec, so most operations that don't change the size or require allocation belong on IdxSet. There is however an existing clone_from method on that type that does the same thing:

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_data_structures/indexed_set/struct.IdxSet.html#method.clone_from

But maybe we should rename that method (IdxSet::clone_from) to overwrite? I wonder if we are having shadowing problems because of Clone::clone_from....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right about the shadowing. Here's the DHAT output:

==60110== -------------------- 1 of 500 --------------------
==60110== max-live:    217,728 in 1,701 blocks
==60110== tot-alloc:   77,545,592 in 1,330,065 blocks (avg size 58.30)
==60110== deaths:      1,330,065, at avg age 2,374,592 (0.00% of prog lifetime)
==60110== acc-ratios:  0.07 rd, 1.51 wr  (5,953,680 b-read, 117,537,792 b-written)
==60110==    at 0x4C2DECF: malloc (in /usr/lib/valgrind/vgpreload_exp-dhat-amd64-linux.so)
==60110==    by 0x7C016B0: alloc (alloc.rs:62)
==60110==    by 0x7C016B0: alloc (alloc.rs:123)
==60110==    by 0x7C016B0: allocate_in<usize,alloc::alloc::Global> (raw_vec.rs:103)
==60110==    by 0x7C016B0: with_capacity<usize> (raw_vec.rs:147)
==60110==    by 0x7C016B0: with_capacity<usize> (vec.rs:362)
==60110==    by 0x7C016B0: to_vec<usize> (slice.rs:167)
==60110==    by 0x7C016B0: to_vec<usize> (slice.rs:369)
==60110==    by 0x7C016B0: clone<usize> (vec.rs:1670)
==60110==    by 0x7C016B0: clone<rustc::mir::Local> (indexed_set.rs:36)
==60110==    by 0x7C016B0: clone_from<rustc_data_structures::indexed_set::IdxSetBuf<rustc::mir::Local>> (clone.rs:131)
==60110==    by 0x7C016B0: rustc_mir::util::liveness::liveness_of_locals (liveness.rs:144)
==60110==    by 0x7B22D4B: compute (liveness.rs:106)
==60110==    by 0x7B22D4B: rustc_mir::borrow_check::nll::compute_regions (mod.rs:101)
==60110==    by 0x7B33381: rustc_mir::borrow_check::do_mir_borrowck (mod.rs:238)

IdxSetBuf implements Clone, and therefore defines clone_from, which is a provided method that calls clone. IdxSet has a clone_from method, which is probably intended to be called, but the IdxSetBuf implementation is called instead.

I think renaming IdxSet::clone_from as IdxSet::overwrite is reasonable. I wonder if there are any other places like this, where the clone_from being called is not the one intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

There aren't many definitions of clone_from, so it wouldn't be hard to audit.

@nikomatsakis nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2018
The current situation is something of a mess.

- `IdxSetBuf` derefs to `IdxSet`.
- `IdxSetBuf` implements `Clone`, and therefore has a provided `clone_from`
  method, which does allocation and so is expensive.
- `IdxSet` has a `clone_from` method that is non-allocating and therefore
  cheap, but this method is not from the `Clone` trait.

As a result, if you have an `IdxSetBuf` called `b`, if you call
`b.clone_from(b2)` you'll get the expensive `IdxSetBuf` method, but if you call
`(*b).clone_from(b2)` you'll get the cheap `IdxSetBuf` method.
`liveness_of_locals()` does the former, presumably unintentionally, and
therefore does lots of unnecessary allocations.

Having a `clone_from` method that isn't from the `Clone` trait is a bad idea in
general, so this patch renames it as `overwrite`. This avoids the unnecessary
allocations in `liveness_of_locals()`, speeding up most NLL benchmarks, the
best by 1.5%. It also means that calls of the form `(*b).clone_from(b2)` can be
rewritten as `b.overwrite(b2)`.
@nnethercote
Copy link
Contributor Author

I have updated. r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

rg 'fn clone_from\b' reports:

src/liballoc/binary_heap.rs:288:    fn clone_from(&mut self, source: &Self) {
src/liballoc/boxed.rs:302:    fn clone_from(&mut self, source: &Box<T>) {
src/liballoc/borrow.rs:173:    fn clone_from(&mut self, source: &Cow<'a, B>) {
src/liballoc/vec.rs:1682:    fn clone_from(&mut self, other: &Vec<T>) {
src/liballoc/string.rs:1688:    fn clone_from(&mut self, source: &Self) {
src/libcore/clone.rs:130:    fn clone_from(&mut self, source: &Self) {
src/libcore/mem.rs:1040:    fn clone_from(&mut self, source: &Self) {
src/librustc_data_structures/indexed_set.rs:236:    pub fn clone_from(&mut self, other: &IdxSet<T>) {

So...seems ok. =)

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 29, 2018

📌 Commit 08683f0 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 29, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jun 30, 2018
…tsakis

Avoid needless allocations in `liveness_of_locals`.

We don't need to replace the heap-allocated bitset, we can just
overwrite its contents.

This speeds up most NLL benchmarks, the best by 1.5%.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jul 1, 2018

⌛ Testing commit 08683f0 with merge e953e46...

bors added a commit that referenced this pull request Jul 1, 2018
Avoid needless allocations in `liveness_of_locals`.

We don't need to replace the heap-allocated bitset, we can just
overwrite its contents.

This speeds up most NLL benchmarks, the best by 1.5%.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jul 1, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing e953e46 to master...

@bors bors merged commit 08683f0 into rust-lang:master Jul 1, 2018
@nnethercote nnethercote deleted the rm-clone_from branch July 1, 2018 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants