-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[experiment] Allocate DepGraph::read_index de-duplication hashset more lazily. #56724
[experiment] Allocate DepGraph::read_index de-duplication hashset more lazily. #56724
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
@bors try |
…try> [experiment] Allocate DepGraph::read_index de-duplication hashset more lazily. Possibly an optimization for `DepGraph::read_index()`. Might need some tweaking. I also want to try a SIMD-based version.
☀️ Test successful - status-travis |
@rust-timer build c81a607 |
Success: Queued c81a607 with parent 8375ab4, comparison URL. |
Finished benchmarking try commit c81a607 |
Looks like the current version has no visible effect. I'll do a SIMD enabled version when I find the time. |
34c3d3c
to
ed37dd0
Compare
@bors try |
⌛ Trying commit ed37dd0decd07fcc058cc5058418cfff8591962b with merge bcc6938d1d1ecf240b1fe74f41d00559e35bdba3... |
The job Click to expand the log.
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 |
💔 Test failed - status-travis |
The job Click to expand the log.
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 |
ed37dd0
to
453141b
Compare
@bors try |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
453141b
to
9ab4bb7
Compare
@bors try |
⌛ Trying commit 9ab4bb7cff6423c7d58cb2152aca73c8793ff86f with merge 79ff98b6c3799f0c9e748102f216ea4c909dd8cc... |
💔 Test failed - status-travis |
The job Click to expand the log.
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 |
9ab4bb7
to
3d610b4
Compare
@bors try |
⌛ Trying commit 3d610b4 with merge b350a5644a4b98b0c16953bc075e40ec5cb2cbd0... |
☀️ Test successful - status-travis |
@rust-timer build b350a5644a4b98b0c16953bc075e40ec5cb2cbd0 |
Success: Queued b350a5644a4b98b0c16953bc075e40ec5cb2cbd0 with parent dd8fc7d, comparison URL. |
Do you have any stats on how many reads a dep node typically gets and how many of these are duplicates? |
I was considering just using an atomic Vec for |
Yes, @wesleywiser collected some data a while ago and even implemented delayed deduplication but it didn't improve performance and increased memory usage. Here's the relevant issue and PR:
I also did some experiments similar to this one but it didn't improve performance either:
I don't expect this to be much of a win, if at all. It's a spare-time project for me, more or less
|
Finished benchmarking try commit b350a5644a4b98b0c16953bc075e40ec5cb2cbd0 |
Yeah, that doesn't seem to make a measurable difference. Bummer. |
Possibly an optimization for
DepGraph::read_index()
. Might need some tweaking. I also want to try a SIMD-based version.