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

Use sparse bitsets instead of dense ones for NLL results #48245

Merged
merged 4 commits into from
Feb 24, 2018

Conversation

spastorino
Copy link
Member

@spastorino spastorino commented Feb 15, 2018

This is for #48170.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2018
@spastorino spastorino force-pushed the sparse_bitsets branch 2 times, most recently from d1d264a to de71da1 Compare February 15, 2018 22:22
@spastorino spastorino changed the title [WIP] use sparse bitsets instead of dense ones for NLL results Use sparse bitsets instead of dense ones for NLL results Feb 15, 2018
@pnkfelix
Copy link
Member

Do we have data about how much this improves memory usage? And what its cost in execution time is, if any?

@spastorino
Copy link
Member Author

spastorino commented Feb 16, 2018

@pnkfelix we should profile this 👍. I just started this as a wip thing and ended being a working solution quickly. I know the code is bad in some parts so I didn't bother until now to run benchs. I think I should do it now and then tidy up the code a bit 😃.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

looks nice!

@@ -260,7 +260,79 @@ impl BitMatrix {
}
}

#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct SparseBitMatrix<I: Idx> {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to use a type I, then I think this should be

SparseBitMatrix<R, C> where R: Idx, C: Idx {
    vector: IndexVec<R, SparseBitSet<C>>
}

/// Create a new `rows x columns` matrix, initially empty.
pub fn new(rows: usize, _columns: usize) -> SparseBitMatrix<I> {
SparseBitMatrix {
vector: vec![SparseBitSet::new(); rows],
Copy link
Contributor

Choose a reason for hiding this comment

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

and this would be IndexVec::from_elem_n(SparseBitSet::new(), rows)

/// `column` to the bitset for `row`.
///
/// Returns true if this changed the matrix, and false otherwise.
pub fn add(&mut self, row: usize, column: usize) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

and this would be row: R, column: C

/// variable. The columns consist of either universal regions or
/// points in the CFG.
#[derive(Clone)]
pub(super) struct RegionValues {
elements: Rc<RegionValueElements>,
matrix: BitMatrix,
matrix: SparseBitMatrix<usize>,
Copy link
Contributor

Choose a reason for hiding this comment

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

then this would be SparseBitMatrix<RegionVid, RegionElementIndex> -- and some casting below would go away

@spastorino
Copy link
Member Author

PR is now ready, I still need to run benchs, etc. Going to do that next.

@spastorino
Copy link
Member Author

spastorino commented Feb 17, 2018

Using steps to reproduce here rust-lang/rust-memory-model#26 I see the following ...

master
    Finished dev [unoptimized + debuginfo] target(s) in 7.24 secs

sparse_bitsets
    Finished dev [unoptimized + debuginfo] target(s) in 7.14 secs

As you can see we have a little gain here, unsure if it's what @nikomatsakis expected or if he expected more :/.

I was algo trying to profile memory valgrind's massif, and I'm getting a segfault, unsure if you have made this run and how with rust ...

[santiago@archlinux syn ((0.12.0))]$ CARGO_INCREMENTAL=0 valgrind --tool=massif cargo +stage1 rustc -v
==7215== Massif, a heap profiler
==7215== Copyright (C) 2003-2017, and GNU GPL'd, by Nicholas Nethercote
==7215== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==7215== Command: cargo +stage1 rustc -v
==7215== 
thread 'main' panicked at 'internal error: entered unreachable code: not all instructions were compiled! found uncompiled instruction: Compiled(Bytes(InstBytes { goto: 12, start: 101, end: 101 }))', target/cargo-home/registry/src/gh.neting.cc-1ecc6299db9ec823/regex-0.2.6/src/compile.rs:794:17
note: Run with `RUST_BACKTRACE=1` for a backtrace.
==7215== 
==7215== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==7215==  Access not within mapped region at address 0x1000000823
==7215==    at 0x63573E: je_huge_dalloc (rtree.h:152)
==7215==    by 0x2D96C3: core::ptr::drop_in_place (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2E409A: regex::compile::Compiler::fill (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2E0B97: regex::compile::Compiler::c (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2E2BEE: regex::compile::Compiler::c_alternate (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2E0B3D: regex::compile::Compiler::c (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2DEA2D: regex::compile::Compiler::compile (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2E9ECB: regex::exec::ExecBuilder::build (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x30208C: regex::re_unicode::Regex::new (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2837BE: rustup_dist::dist::PartialToolchainDesc::from_str (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2537DC: rustup::config::Cfg::resolve_toolchain (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x2414B3: rustup::toolchain::Toolchain::from (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x252536: rustup::config::Cfg::create_command_for_toolchain (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x1B39F9: rustup_init::run_rustup (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x1B16AF: rustup_init::main (in /home/santiago/.cargo/bin/cargo)
==7215==    by 0x61078E: __rust_maybe_catch_panic (lib.rs:101)
==7215==    by 0x6082BB: std::rt::lang_start (panicking.rs:459)
==7215==    by 0x58A8F49: (below main) (in /usr/lib/libc-2.26.so)
==7215==  If you believe this happened as a result of a stack
==7215==  overflow in your program's main thread (unlikely but
==7215==  possible), you can try to increase the size of the
==7215==  main thread stack using the --main-stacksize= flag.
==7215==  The main thread stack size used in this run was 8388608.
==7215== 
Segmentation fault (core dumped)

@nikomatsakis
Copy link
Contributor

As you can see we have a little gain here, unsure if it's what @nikomatsakis expected or if he expected more :/.

I did not expect a big impact here on this particular test case. But I think this will be more important on larger test cases. And the fact that we saw a small improvement means we're not paying a price for the improved memory usage.

pub fn merge(&mut self, read: R, write: R) -> bool {
let mut changed = false;

let bit_set_read = self.vector[read].clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

to avoid the clone, add a helper function like this one. Maybe we should add that somewhere else, if we don't have it already.

let bit_set_a = &self.vector[a];
let bit_set_b = &self.vector[b];

for a_val in bit_set_a.iter().filter(|x| bit_set_b.contains(*x)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I bet this method can just be removed. But if not, it could be made faster by adding a method intersection on SparseBitset. I guess it would work something like this:

let mut result = SparseBitset::new();
for (key, value) in self.map {
    match other.map.get(key) {
        None => { /* other has no values with these key */ }
        Some(other_value) {
            if value & other_value != 0 {
                result.map.insert(key, value & other_value);
            }
        }
    }
}

This of course generates a new sparse-bitset.

impl<T> SliceExt<T> for [T] {
type Item = T;

fn pick2(&mut self, a: usize, b: usize) -> (&mut T, &mut T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment here?

Something like:


Return mutable references to two distinct elements, a and b. Panics if a == b.


Also, maybe we could call it get2_mut? (similar to get_mut?)

Copy link
Contributor

@nikomatsakis nikomatsakis Feb 19, 2018

Choose a reason for hiding this comment

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

At least I think we should add a _mut suffix

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not call it get2 since it doesn't return an Option (as get does).

pub trait SliceExt<T> {
type Item;

fn pick2(&mut self, a: usize, b: usize) -> (&mut T, &mut T);
Copy link
Contributor

Choose a reason for hiding this comment

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

Either:

  • This should be &mut Self::Item and there is no need for the T parameter

Or:

  • Remove the type Item

/// variable. The columns consist of either universal regions or
/// points in the CFG.
#[derive(Clone)]
pub(super) struct RegionValues {
elements: Rc<RegionValueElements>,
matrix: BitMatrix,
matrix: SparseBitMatrix<RegionVid, RegionElementIndex>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to say, pushing up the "index types" is nice.

@spastorino spastorino force-pushed the sparse_bitsets branch 4 times, most recently from f3af3f7 to d5811f7 Compare February 19, 2018 18:03
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 19, 2018

📌 Commit d5811f7 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2018
@eddyb
Copy link
Member

eddyb commented Feb 22, 2018

Could this use the chunked API from #47575 (comment)?

if read != write {
let (bit_set_read, bit_set_write) = self.vector.pick2_mut(read, write);

for read_val in bit_set_read.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be made faster with chunks, right? (this is going a bit at a time, right, not a word at a time?)

Copy link
Contributor

Choose a reason for hiding this comment

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

(I think @spastorino and I talked about that already at some point...)

// it does not always merge an entire row. That would
// complicate causal tracking though.
debug!(
"add_universal_regions_outlived_by(from_region={:?}, to_region={:?})",
from_region,
to_region
from_region, to_region
);
let mut changed = false;
for elem in self.elements.all_universal_region_indices() {
Copy link
Contributor

Choose a reason for hiding this comment

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

this could probably use chunks

@@ -238,7 +238,7 @@ impl RegionValues {
where
F: FnOnce(&CauseMap) -> Cause,
{
if self.matrix.add(r.index(), i.index()) {
if self.matrix.add(r, i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we'd have to look at the callers of these add methods...

@eddyb
Copy link
Member

eddyb commented Feb 22, 2018

(from IRC) SparseBitMatrix could also have a chunked API, e.g. row_chunks(row).

Unverified

This user has not yet uploaded their public signing key.
@nikomatsakis
Copy link
Contributor

@eddyb I'm inclined to land some variant of these changes, but otherwise let you revise and merge this with your own API when that is making progress. Seem reasonable?

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2018

📌 Commit 6a74615 has been approved by nikomatsakis

@eddyb
Copy link
Member

eddyb commented Feb 23, 2018

@nikomatsakis Yeah. Context is @spastorino explicitly asked for me to leave a comment on GitHub.

@whoisj
Copy link

whoisj commented Feb 23, 2018

As an aside, @spastorino if you format your pull request description with a line that says "Resolves #48170" instead of "this is for", GitHub will automagically link the PR and issue, and resolve the issue if/when the PR merges. Just a helpful usage hint. 😄

@spastorino
Copy link
Member Author

@whoisj yeah, I added that in the commit message ff9eb56 for exactly that reason. Anyway, good to know that's possible (and maybe better) to do it on the PR.

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 23, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…tsakis

Use sparse bitsets instead of dense ones for NLL results

This is for rust-lang#48170.

r? @nikomatsakis
bors added a commit that referenced this pull request Feb 23, 2018
Rollup of 12 pull requests

- Successful merges: #47933, #48072, #48083, #48123, #48157, #48219, #48221, #48245, #48429, #48436, #48438, #48472
- Failed merges:
bors added a commit that referenced this pull request Feb 24, 2018
Rollup of 12 pull requests

- Successful merges: #47933, #48072, #48083, #48123, #48157, #48219, #48221, #48245, #48429, #48436, #48438, #48472
- Failed merges:
@bors bors merged commit 6a74615 into rust-lang:master Feb 24, 2018
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.

7 participants