Merge `bitvec.rs` and `indexed_set.rs` by nnethercote · Pull Request #54286 · rust-lang/rust

nnethercote · 2018-09-17T09:31:53Z

Because it's not good to have two separate implementations. Also, I will combine the best parts of each to improve NLL memory usage on some benchmarks significantly.

rust-highfive · 2018-09-17T09:32:04Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

nnethercote · 2018-09-17T09:40:34Z

Also, I will combine the best parts of each to improve NLL memory usage on some benchmarks significantly.

See #52028 (comment) for some measurements.

nnethercote · 2018-09-17T10:07:48Z

@bors try

bors · 2018-09-17T10:08:02Z

⌛ Trying commit 9935d42 with merge fc377bd...

Merge `bitvec.rs` and `indexed_set.rs` Because it's not good to have two separate implementations. Also, I will combine the best parts of each to improve NLL memory usage on some benchmarks significantly.

src/librustc_data_structures/bit_set.rs

pnkfelix · 2018-09-17T10:35:24Z

src/librustc_data_structures/bit_set.rs

    /// (i.e., if any bits were removed).
    pub fn intersect(&mut self, other: &BitSet<T>) -> bool {
-        bitwise(self.words_mut(), other.words(), &Intersect)
+        bitwise(self.words_mut(), other.words(), |a, b| { a & b })


hmm I guess I can hope this always gets inlined even on debug builds (which I believe was a property of the former architecture here...)

pnkfelix · 2018-09-17T10:40:37Z

r=me; I'd like some of the deleted documentation to be preserved, but I'm not going to block landing the PR on completion of that task. I'll let @nnethercote decide whether to port over the docs or not as I asked in a comment in my review.

rust-highfive · 2018-09-17T10:51:50Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:52:49] ....................................................................................................
[00:52:51] .....................................................i..............................................
[00:52:54] ....................................................................................................
[00:52:57] ....................................................................................................
[00:53:00] .iiiiiiiii..........................................................................................
[00:53:06] ....................................................................................................
[00:53:09] ..................................................................................i.................
[00:53:12] ....................................................................................................
[00:53:15] ....................................i.i..ii.........................................................
---
[01:17:03] travis_fold:start:test_stage1-rustc_data_structures
travis_time:start:test_stage1-rustc_data_structures
Testing rustc_data_structures stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:17:04]    Compiling rustc_data_structures v0.0.0 (file:///checkout/src/librustc_data_structures)
[01:17:05] error[E0599]: no method named `merge` found for type `bit_set::BitSet<usize>` in the current scope
[01:17:05]    --> librustc_data_structures/bit_set.rs:859:18
[01:17:05]     |
[01:17:05] 28  | pub struct BitSet<T: Idx> {
[01:17:05]     | ------------------------- method `merge` not found for this
[01:17:05] ...
[01:17:05] 859 |     assert!(set1.merge(&set2));
[01:17:05] 
[01:17:05] 
[01:17:05] error[E0599]: no method named `merge` found for type `bit_set::BitSet<usize>` in the current scope
[01:17:05]    --> librustc_data_structures/bit_set.rs:860:19
[01:17:05]     |
[01:17:05] 28  | pub struct BitSet<T: Idx> {
[01:17:05]     | ------------------------- method `merge` not found for this
[01:17:05] ...
[01:17:05] 860 |     assert!(!set1.merge(&set2));
[01:17:05] 
[01:17:05] ...
[01:17:05] ...
[01:17:05] 921 |     let intersection = matrix.intersection(2, 65);
[01:17:05]     |
[01:17:05]     = help: did you mean `intersect_rows`?
[01:17:05] 
[01:17:05] 
[01:17:05] error[E0599]: no method named `merge` found for type `bit_set::BitMatrix<usize, usize>` in the current scope
[01:17:05]    --> librustc_data_structures/bit_set.rs:932:12
[01:17:05]     |
[01:17:05] 560 | pub struct BitMatrix<R: Idx, C: Idx> {
[01:17:05]     | ------------------------------------ method `merge` not found for this
[01:17:05] ...
[01:17:05] 932 |     matrix.merge(3, 5);
[01:17:05] 
[01:17:05] 
[01:17:05] error[E0599]: no method named `merge` found for type `bit_set::SparseBitMatrix<usize, usize>` in the current scope
[01:17:05]    --> librustc_data_structures/bit_set.rs:974:12
[01:17:05]     |
[01:17:05] 677 | / pub struct SparseBitMatrix<R, C>
[01:17:05] 678 | | where
[01:17:05] 679 | |     R: Idx,
[01:17:05] 680 | |     C: Idx,
[01:17:05] ...   |
[01:17:05] 683 | |     rows: IndexVec<R, Option<BitSet<C>>>,
[01:17:05] 684 | | }
[01:17:05]     | |_- method `merge` not found for this
[01:17:05] ...
[01:17:05] 974 |       matrix.merge(3, 5);
[01:17:05] 
travis_time:end:20c3dbc8:start=1537176877052734143,finish=1537181504494778294,duration=4627442044151

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

kennytm · 2018-09-17T11:23:57Z

@bors r- CI failed.

bors · 2018-09-17T14:08:02Z

💥 Test timed out

Currently we have two files implementing bitsets (and 2D bit matrices). This commit combines them into one, taking the best features from each. This involves renaming a lot of things. The high level changes are as follows. - bitvec.rs --> bit_set.rs - indexed_set.rs --> (removed) - BitArray + IdxSet --> BitSet (merged, see below) - BitVector --> GrowableBitSet - {,Sparse,Hybrid}IdxSet --> {,Sparse,Hybrid}BitSet - BitMatrix --> BitMatrix - SparseBitMatrix --> SparseBitMatrix The changes within the bitset types themselves are as follows. ``` OLD OLD NEW BitArray<C> IdxSet<T> BitSet<T> -------- ------ ------ grow - grow new - (remove) new_empty new_empty new_empty new_filled new_filled new_filled - to_hybrid to_hybrid clear clear clear set_up_to set_up_to set_up_to clear_above - clear_above count - count contains(T) contains(&T) contains(T) contains_all - superset is_empty - is_empty insert(T) add(&T) insert(T) insert_all - insert_all() remove(T) remove(&T) remove(T) words words words words_mut words_mut words_mut - overwrite overwrite merge union union - subtract subtract - intersect intersect iter iter iter ``` In general, when choosing names I went with: - names that are more obvious (e.g. `BitSet` over `IdxSet`). - names that are more like the Rust libraries (e.g. `T` over `C`, `insert` over `add`); - names that are more set-like (e.g. `union` over `merge`, `superset` over `contains_all`, `domain_size` over `num_bits`). Also, using `T` for index arguments seems more sensible than `&T` -- even though the latter is standard in Rust collection types -- because indices are always copyable. It also results in fewer `&` and `*` sigils in practice.

`BitwiseOperator` is an unnecessarily low-level thing. This commit replaces it with `BitSetOperator`, which works on `BitSet`s instead of words. Within `bit_set.rs`, the commit eliminates `Intersect`, `Union`, and `Subtract` by instead passing a function to `bitwise()`.

- Rename `BitSet::data` and `BitMatrix::vector` as `words`, because that's what they are. - Remove `BitSet::words_mut()`, which is no longer necessary. - Better distinguish multiple meanins of "word", i.e. "word index" vs "word ref" vs "word" (i.e. the value itself).

nnethercote · 2018-09-17T22:33:22Z

I have added comments about the index types, and fixed the problems with the tests.

r=pnkfelix

nnethercote · 2018-09-17T22:34:42Z

@bors r=pnkfelix

bors · 2018-09-17T22:34:43Z

📌 Commit 53589b7 has been approved by pnkfelix

eddyb · 2018-09-18T03:09:11Z

@bors p=3 (blocks some NLL memory usage work, according to IRC)

bors · 2018-09-18T03:52:47Z

⌛ Testing commit 53589b7 with merge b80cb47...

Merge `bitvec.rs` and `indexed_set.rs` Because it's not good to have two separate implementations. Also, I will combine the best parts of each to improve NLL memory usage on some benchmarks significantly.

bors · 2018-09-18T06:24:50Z

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

@pnkfelix

…x, r=pnkfelix Use `HybridBitSet` in `SparseBitMatrix`. This fixes most of the remaining NLL memory regression. r? @pnkfelix, because you reviewed #54286. cc @nikomatsakis, because NLL cc @Mark-Simulacrum, because this removes `array_vec.rs` cc @lqd, because this massively improves `unic-ucd-name`, and probably other public crates

@pnkfelix

…x, r=pnkfelix Use `HybridBitSet` in `SparseBitMatrix`. This fixes most of the remaining NLL memory regression. r? @pnkfelix, because you reviewed #54286. cc @nikomatsakis, because NLL cc @Mark-Simulacrum, because this removes `array_vec.rs` cc @lqd, because this massively improves `unic-ucd-name`, and probably other public crates

rust-highfive assigned pnkfelix Sep 17, 2018

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

nnethercote mentioned this pull request Sep 17, 2018

html5ever in the rustc-perf repository is memory-intensive #52028

Closed

pnkfelix reviewed Sep 17, 2018

View reviewed changes

src/librustc_data_structures/bit_set.rs Show resolved Hide resolved

pnkfelix reviewed Sep 17, 2018

View reviewed changes

pnkfelix 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 Sep 17, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 17, 2018

nnethercote added 3 commits September 18, 2018 07:08

nnethercote force-pushed the BitSet branch from 9935d42 to 53589b7 Compare September 17, 2018 22:32

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 Sep 17, 2018

bors merged commit 53589b7 into rust-lang:master Sep 18, 2018

nnethercote deleted the BitSet branch September 18, 2018 06:27

nnethercote mentioned this pull request Sep 18, 2018

Use HybridBitSet in SparseBitMatrix. #54318

Merged

bors mentioned this pull request Sep 18, 2018

rustc_codegen_llvm: traitification of LLVM-specific CodegenCx and Builder methods #54012

Closed

Uh oh!

Conversation

nnethercote commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Sep 17, 2018

Uh oh!

nnethercote commented Sep 17, 2018

Uh oh!

nnethercote commented Sep 17, 2018

Uh oh!

bors commented Sep 17, 2018

Uh oh!

Uh oh!

pnkfelix Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

pnkfelix commented Sep 17, 2018

Uh oh!

rust-highfive commented Sep 17, 2018

Uh oh!

kennytm commented Sep 17, 2018

Uh oh!

bors commented Sep 17, 2018

Uh oh!

nnethercote commented Sep 17, 2018

Uh oh!

nnethercote commented Sep 17, 2018

Uh oh!

bors commented Sep 17, 2018

Uh oh!

eddyb commented Sep 18, 2018

Uh oh!

bors commented Sep 18, 2018

Uh oh!

bors commented Sep 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nnethercote commented Sep 17, 2018 •

edited

Loading