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

librustc_data_structures: Speedup union of sparse and dense hybrid set #61020

Merged
merged 3 commits into from Jun 22, 2019

Conversation

Projects
None yet
7 participants
@HeroicKatora
Copy link
Contributor

commented May 21, 2019

This optimization speeds up the union of a hybrid bitset when that
switches it from a sparse representation to a dense bitset. It now
clones the dense bitset and integrate only the spare elements instead of
densifying the sparse bitset, initializing all elements, and then a
union on two dense bitset, touching all words a second time.

It's not completely certain if the added complexity is worth it but I would
like to hear some feedback in any case. Benchmark results from my machine:

Now:  bit_set::union_hybrid_sparse_to_dense ... bench:          72 ns/iter (+/- 5)
Previous: bit_set::union_hybrid_sparse_to_dense ... bench:          90 ns/iter (+/- 6)

This being the second iteration of trying to improve the speed, since I missed the return value in the first, and forgot to run the relevant tests. Oops.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Centril

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

⌛️ Trying commit 3d67c2d with merge b7a0ff7...

bors added a commit that referenced this pull request May 22, 2019

Auto merge of #61020 - HeroicKatora:master, r=<try>
librustc_data_structures: Speedup union of sparse and dense hybrid set

This optimization speeds up the union of a hybrid bitset when that
switches it from a sparse representation to a dense bitset. It now
clones the dense bitset and integrate only the spare elements instead of
densifying the sparse bitset, initializing all elements, and then a
union on two dense bitset, touching all words a second time.

It's not completely certain if the added complexity is worth it but I would
like to hear some feedback in any case. Benchmark results from my machine:

```
Now:  bit_set::union_hybrid_sparse_to_dense ... bench:          72 ns/iter (+/- 5)
Previous: bit_set::union_hybrid_sparse_to_dense ... bench:          90 ns/iter (+/- 6)
```

This being the second iteration of trying to improve the speed, since I missed the return value in the first, and forgot to run the relevant tests. Oops.
@bors

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

☀️ Try build successful - checks-travis
Build commit: b7a0ff7

@Centril

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 22, 2019

Success: Queued b7a0ff7 with parent 119bbc2, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented May 22, 2019

Finished benchmarking try commit b7a0ff7: comparison url

@HeroicKatora

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Doesn't really seem conclusive either way, maybe the benchmark is not as representative of the usage of the operation as I thought. I'll investigate

HeroicKatora added some commits May 21, 2019

Improve union of sparse and dense hybrid set
This optimization speeds up the union of a hybrid bitset when that
switches it from a sparse representation to a dense bitset. It now
clones the dense bitset and integrate only the spare elements instead of
densifying the sparse bitset, initializing all elements, and then a
union on two dense bitset, touching all words a second time.
Add documentation on the reasoning
Explains the thought process behind adding the union algorithm and
discusses the alternative and heuristic behind.

@HeroicKatora HeroicKatora force-pushed the HeroicKatora:master branch from 3d67c2d to 3f28811 May 22, 2019

@HeroicKatora

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@Centril Can we try again?

@Centril

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@bors try

"Do, or do not. There is no try." -- Yoda

@bors

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

⌛️ Trying commit 3f28811 with merge 9d4d0f3...

bors added a commit that referenced this pull request May 23, 2019

Auto merge of #61020 - HeroicKatora:master, r=<try>
librustc_data_structures: Speedup union of sparse and dense hybrid set

This optimization speeds up the union of a hybrid bitset when that
switches it from a sparse representation to a dense bitset. It now
clones the dense bitset and integrate only the spare elements instead of
densifying the sparse bitset, initializing all elements, and then a
union on two dense bitset, touching all words a second time.

It's not completely certain if the added complexity is worth it but I would
like to hear some feedback in any case. Benchmark results from my machine:

```
Now:  bit_set::union_hybrid_sparse_to_dense ... bench:          72 ns/iter (+/- 5)
Previous: bit_set::union_hybrid_sparse_to_dense ... bench:          90 ns/iter (+/- 6)
```

This being the second iteration of trying to improve the speed, since I missed the return value in the first, and forgot to run the relevant tests. Oops.
@bors

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

☀️ Try build successful - checks-travis
Build commit: 9d4d0f3

@Centril

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@rust-timer

This comment has been minimized.

Copy link

commented May 23, 2019

Success: Queued 9d4d0f3 with parent 11f01bf, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented May 23, 2019

Finished benchmarking try commit 9d4d0f3: comparison url

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Ping from triage @matthewjasper, this is waiting on your review. Feel free to assign someone else if you don't have time though.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

📌 Commit 3f28811 has been approved by matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

⌛️ Testing commit 3f28811 with merge 4a365a2...

bors added a commit that referenced this pull request Jun 22, 2019

Auto merge of #61020 - HeroicKatora:master, r=matthewjasper
librustc_data_structures: Speedup union of sparse and dense hybrid set

This optimization speeds up the union of a hybrid bitset when that
switches it from a sparse representation to a dense bitset. It now
clones the dense bitset and integrate only the spare elements instead of
densifying the sparse bitset, initializing all elements, and then a
union on two dense bitset, touching all words a second time.

It's not completely certain if the added complexity is worth it but I would
like to hear some feedback in any case. Benchmark results from my machine:

```
Now:  bit_set::union_hybrid_sparse_to_dense ... bench:          72 ns/iter (+/- 5)
Previous: bit_set::union_hybrid_sparse_to_dense ... bench:          90 ns/iter (+/- 6)
```

This being the second iteration of trying to improve the speed, since I missed the return value in the first, and forgot to run the relevant tests. Oops.
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: matthewjasper
Pushing 4a365a2 to master...

@bors bors added the merged-by-bors label Jun 22, 2019

@bors bors merged commit 3f28811 into rust-lang:master Jun 22, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.