Use sparse bitsets instead of dense ones for NLL results

[spastorino]

This is for #48170.

r? @nikomatsakis

[pnkfelix]

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

[spastorino]

@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 😃.

[nikomatsakis]

looks nice!

[nikomatsakis]

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>>
}

[nikomatsakis]

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

[nikomatsakis]

and this would be row: R, column: C

[nikomatsakis]

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

[spastorino]

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

[spastorino]

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/github.com-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]

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.

[nikomatsakis]

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.

[nikomatsakis]

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.

[nikomatsakis]

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?)

[nikomatsakis]

At least I think we should add a _mut suffix

[nikomatsakis]

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

[nikomatsakis]

Either:

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

Or:

• Remove the type Item

[nikomatsakis]

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

[nikomatsakis]

@bors r+

[bors]

📌 Commit d5811f7 has been approved by nikomatsakis

[eddyb]

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

[nikomatsakis]

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

[nikomatsakis]

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

[nikomatsakis]

this could probably use chunks

[nikomatsakis]

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

[eddyb]

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

[nikomatsakis]

@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?

[nikomatsakis]

@bors r+

[bors]

📌 Commit 6a74615 has been approved by nikomatsakis

[eddyb]

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

[whoisj]

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]

@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.