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

Use HybridBitSet in SparseBitMatrix. #54318

Merged
merged 3 commits into from
Sep 19, 2018
Merged

Use HybridBitSet in SparseBitMatrix. #54318

merged 3 commits into from
Sep 19, 2018

Conversation

nnethercote
Copy link
Contributor

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

@nnethercote
Use elem instead of bit consistently for arguments.
c42765a
@nnethercote
Remove array_vec.rs.
687cc29 
`SparseBitSet` is the only remaining user of `ArrayVec`. This commit
switches it to using `SmallVec`, and removes `array_vec.rs`.

Why the switch? Although `SparseBitSet` is size-limited and doesn't need
the ability to spill to the heap, `SmallVec` has many more features than
`ArrayVec`. In particular, it's now possible to keep `SparseBitSet`'s
elements in sorted order, which gives in-order iteration, which is a
requirement for the next commit.
@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

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

This comment has been minimized.

Sign in to view
@nnethercote
Use HybridBitSet for rows within SparseBitMatrix.
154be2c 
This requires adding a few extra methods to `HybridBitSet`. (These are
tested in a new unit test.)

This commit reduces the `max-rss` for `nll-check` builds of `html5ever`
by 46%, `ucd` by 45%, `clap-rs` by 23%, `inflate` by 14%. And the
results for the `unic-ucd-name` crate are even more impressive: a 21%
reduction in instructions, a 60% reduction in wall-time, a 96%
reduction in `max-rss`, and a 97% reduction in faults!

Fixes rust-lang#52028.
@nnethercote nnethercote force-pushed the use-HybridBitSet-in-SparseBitMatrix branch from b457ed0 to 154be2c Compare September 18, 2018 06:42
@nnethercote
Copy link
Contributor Author

The submodules changes were accidental, and I have reverted them.

@nnethercote
Copy link
Contributor Author

[00:02:28] error: the lock file needs to be updated but --locked was passed to prevent this

The second commit is probably the cause of this, because it removes array_vec.rs... but I don't know what needs to be done.

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 18, 2018

⌛ Trying commit 154be2c with merge c243f30671409bce0ef56f65b318efac3c0012ff...

@bors
Copy link
Contributor

bors commented Sep 18, 2018

☀️ Test successful - status-travis
State: approved= try=True

@nnethercote
Copy link
Contributor Author

@rust-timer build c243f30671409bce0ef56f65b318efac3c0012ff

@rust-timer
Copy link
Collaborator

Success: Queued c243f30671409bce0ef56f65b318efac3c0012ff with parent b80cb47, comparison URL.

@pnkfelix
Copy link
Member

Looks great to me.

The dummy bitsets are unfortunate, but I'd rather land this first and then after that, maybe I will investigate revising the coding patterns to get rid of them.

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 18, 2018

📌 Commit 154be2c has been approved by pnkfelix

@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 Sep 18, 2018
@pnkfelix
Copy link
Member

ah, I guess it would be prudent to wait until the results are back from rust-timer.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 18, 2018
@pnkfelix pnkfelix 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 18, 2018
@nnethercote
Copy link
Contributor Author

I expect the rust-timer results will be much the same as what I got locally, but sure, we can wait.

@lqd
Copy link
Member

lqd commented Sep 18, 2018

perf results are ready, awesome improvements for max-rss as expected 🎉

@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 18, 2018

📌 Commit 154be2c has been approved by pnkfelix

@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2018
@rust-highfive
Copy link
Collaborator

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. 
✓ uploaded script
travis_fold:end:step_upload_script
travis_fold:start:worker_info
Worker information
hostname: 8693e843-d64a-4db0-a622-2d8a987f7042@1.production-2-worker-org-gce-zm1x
instance: travis-job-a76ee0cc-3f8b-45e9-afbf-a3b90ac37f86 travis-ci-connie-trusty-1512502258-986baf0 (via amqp)
startup: 48.856560272s
travis_fold:end:worker_info
travis_fold:start:system_info
---
Attempting to download s3://rust-lang-ci-sccache2/docker/fb846323fae811c75bb1af129bb81e3ccd01260054711cf61af1fd9e28020555f48a094c896bb167411b9470d64a9b0660359d4933a1576f31626975541a2dc0
[00:19:40] Attempting with retry: curl -f -L -C - -o /tmp/rustci_docker_cache https://s3-us-west-1.amazonaws.com/rust-lang-ci-sccache2/docker/fb846323fae811c75bb1af129bb81e3ccd01260054711cf61af1fd9e28020555f48a094c896bb167411b9470d64a9b0660359d4933a1576f31626975541a2dc0
[00:19:40]   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
[00:19:40]                                  Dload  Upload   Total   Spent    Left  Speed
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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)

@pietroalbini
Copy link
Member

@bors retry #40474

@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 Sep 18, 2018
@bors
Copy link
Contributor

bors commented Sep 18, 2018

⌛ Testing commit 154be2c with merge 64c407e0bd7c91a9feb4666a90075a1c650460ed...

@memoryruins memoryruins added the A-NLL Area: Non-lexical lifetimes (NLL) label Sep 18, 2018
@bors
Copy link
Contributor

bors commented Sep 18, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 18, 2018
@rust-highfive
Copy link
Collaborator

The job dist-x86_64-linux-alt 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. 
✓ uploaded script
travis_fold:end:step_upload_script
travis_fold:start:worker_info
Worker information
hostname: 7b607e1c-491c-44fc-9630-8fbd518d037d@1.production-2-worker-org-gce-zfk4
instance: travis-job-a5986c39-ca1a-4f1c-816d-0ee5983b21f7 travis-ci-connie-trusty-1512502258-986baf0 (via amqp)
startup: 27.59767184s
travis_fold:end:worker_info
travis_fold:start:system_info
---
[02:56:25]    Compiling clippy_lints v0.0.212 (file:///checkout/src/tools/clippy/clippy_lints)
[02:56:26]    Compiling cargo_metadata v0.6.0
[02:56:38]    Compiling url v1.7.1
[02:58:05] [RUSTC-TIMING] clippy_lints test:false 100.281
The job exceeded the maximum time limit for jobs, and has been terminated.

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)

@shepmaster
Copy link
Member

@bors retry

@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 Sep 18, 2018
@bors
Copy link
Contributor

bors commented Sep 18, 2018

⌛ Testing commit 154be2c with merge 2e51a80...

bors added a commit that referenced this pull request Sep 18, 2018
@bors
Auto merge of #54318 - nnethercote:use-HybridBitSet-in-SparseBitMatri…
2e51a80 
…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
@nnethercote
Copy link
Contributor Author

nnethercote commented Sep 18, 2018 •
edited
Loading

The dummy bitsets are unfortunate, but I'd rather land this first and then after that, maybe I will investigate revising the coding patterns to get rid of them.

I'd love to know how to avoid them. I asked about this for a similar example in /r/rust and the answers all involved repeated matches and unreachable!()s.

@bors
Copy link
Contributor

bors commented Sep 19, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 19, 2018
@nnethercote
Copy link
Contributor Author

AFAICT, the i686-pc-windows-gnu machine hit the 3 hr limit. (The x86_64-pc-windows-gnu build took 2h58m.)

@bors retry, I guess

@nnethercote
Copy link
Contributor Author

@bors retry

@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 Sep 19, 2018
@bors
Copy link
Contributor

bors commented Sep 19, 2018

⌛ Testing commit 154be2c with merge ff6422d...

bors added a commit that referenced this pull request Sep 19, 2018
@bors
Auto merge of #54318 - nnethercote:use-HybridBitSet-in-SparseBitMatri…
ff6422d 
…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
@bors
Copy link
Contributor

bors commented Sep 19, 2018

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

@bors bors merged commit 154be2c into rust-lang:master Sep 19, 2018
@nnethercote nnethercote deleted the use-HybridBitSet-in-SparseBitMatrix branch September 19, 2018 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@pnkfelix pnkfelix

Labels
A-NLL Area: Non-lexical lifetimes (NLL) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@nnethercote @rust-highfive @bors @rust-timer @pnkfelix @lqd @nikomatsakis @pietroalbini @shepmaster @memoryruins