Persistent data structures by im-rs #6336
Conversation
|
I added some comments, and some variable names. Does that help? |
|
Yeah, it does, thank you! I had assumed that the increment was relevant to the ordering. If it's just a question of not making elements disappear, why not just have a wrapper with a custom |
|
Outside of that, I'm convinced by your arguments so I'm happy to land this. 😄 |
|
... let me try that. BRB. |
|
That is what I remembered, lots of the test fail to pass. According to Ord "Implementations of PartialEq, PartialOrd, and Ord must agree with each other." So I changed |
|
Yeah |
|
It also needs to be deterministic, so we can't use the memory address to duplicate. |
|
This looks pretty awesome to me, having drop-in replacements is indeed really nice! Out of curiosity, have you taken a look at the perf as-is today? (just to make sure we're not accidentally having a regression) |
|
According to the discussion in bodil/im-rs#26 I have had a version of this branch since Jun 21. I have at various times done detailed analysis of micro benchmark performance and experiments with cargo performance. Overall both However I have not rerun the perf recently, if you have something you would like me to try let me know. (Servo is ridiculously slow on my computer, so not that.) |
|
@bors: r+ Ok that sounds reasonable to me! I definitely looks like you've been far more thorough here than I would have been :) |
|
📌 Commit 1699a5b has been approved by |
Persistent data structures by im-rs There has been a long standing "TODO: We should switch to persistent hash maps if we can" in the resolver. This PR introduces a dependency on [im-rs](bodil/im-rs#26) to provide persistent hash maps. It then uses `im_rc::OrdSet` to store the priority list of dependencies, instead of `std::BinaryHeap`, as cloning the `Heap` was one of the most expensive things we did. In Big O terms these changes are very big wins, in practice the improvement is small. This is do to a number of factors like, `std::collections` are very fast, N is usually only in the hundreds, we only clone when the borrow checker insists on it, and we use `RC` and other tricks to avoid cloning. I would advocate that we accept this PR, as: - Things are only going to get more complicated in the future. It would be nice to have Big O on our side. - When developing a new feature finding each place to add `RC` should be an afterthought not absolutely required before merge. (cc #6129 witch is stuck on this kind of per tick performance problem as well as other things). - As the code gets more complex, making sure we never break any of the tricks becomes harder. It would be easier to work on this if such mistakes are marginal instead of show stoppers. (cc #5130 a bug report of one of these bing removed and affecting `./x.py build` enough to trigger an investigation). - The resolver is already very complicated with a lot of moving parts to keep in your head at the same time, this will only get worse as we add features. There are a long list of tricks that are *critical* before and `~5%` after, it would be nice to consider if each one is worth the code complexity. (I can list some of the ones I have tried to remove but are still small wins, if that would help the discussion). Overall, I think it is worth doing now, but can see that if is not a slam dunk.
|
☀️ Test successful - status-appveyor, status-travis |
Update cargo 14 commits in b3d0b2e545b61d4cd08096911724b7d49d213f73..1ff5975b96b3d395bb962394596998dfb485f793 2018-11-15 19:13:04 +0000 to 2018-11-25 14:59:12 +0000 - Intern SourceId (rust-lang/cargo#6342) - Tweak Layout to allow for non json file targets with internal "." (rust-lang/cargo#6255) - Correct Target Directory command-line option (rust-lang/cargo#6343) - Persistent data structures by im-rs (rust-lang/cargo#6336) - Move command prelude into main library (rust-lang/cargo#6335) - Distinguish custom build invocations (rust-lang/cargo#6331) - Allow crate_type=bin examples to run (rust-lang/cargo#6330) - Make verify-project honour unstable features (rust-lang/cargo#6326) - Make autodiscovery disable inferred targets (rust-lang/cargo#6329) - Add `c` alias for `check` (rust-lang/cargo#6218) - Allow user aliases to override built-in aliases (rust-lang/cargo#6259) - Fix renaming directory project using build scripts with cross-compiling. (rust-lang/cargo#6328) - Fix add_plugin_deps-related tests. (rust-lang/cargo#6327) - Add a glossary. (rust-lang/cargo#6321)
Update cargo 14 commits in b3d0b2e545b61d4cd08096911724b7d49d213f73..1ff5975b96b3d395bb962394596998dfb485f793 2018-11-15 19:13:04 +0000 to 2018-11-25 14:59:12 +0000 - Intern SourceId (rust-lang/cargo#6342) - Tweak Layout to allow for non json file targets with internal "." (rust-lang/cargo#6255) - Correct Target Directory command-line option (rust-lang/cargo#6343) - Persistent data structures by im-rs (rust-lang/cargo#6336) - Move command prelude into main library (rust-lang/cargo#6335) - Distinguish custom build invocations (rust-lang/cargo#6331) - Allow crate_type=bin examples to run (rust-lang/cargo#6330) - Make verify-project honour unstable features (rust-lang/cargo#6326) - Make autodiscovery disable inferred targets (rust-lang/cargo#6329) - Add `c` alias for `check` (rust-lang/cargo#6218) - Allow user aliases to override built-in aliases (rust-lang/cargo#6259) - Fix renaming directory project using build scripts with cross-compiling. (rust-lang/cargo#6328) - Fix add_plugin_deps-related tests. (rust-lang/cargo#6327) - Add a glossary. (rust-lang/cargo#6321)
Update cargo 14 commits in b3d0b2e545b61d4cd08096911724b7d49d213f73..1ff5975b96b3d395bb962394596998dfb485f793 2018-11-15 19:13:04 +0000 to 2018-11-25 14:59:12 +0000 - Intern SourceId (rust-lang/cargo#6342) - Tweak Layout to allow for non json file targets with internal "." (rust-lang/cargo#6255) - Correct Target Directory command-line option (rust-lang/cargo#6343) - Persistent data structures by im-rs (rust-lang/cargo#6336) - Move command prelude into main library (rust-lang/cargo#6335) - Distinguish custom build invocations (rust-lang/cargo#6331) - Allow crate_type=bin examples to run (rust-lang/cargo#6330) - Make verify-project honour unstable features (rust-lang/cargo#6326) - Make autodiscovery disable inferred targets (rust-lang/cargo#6329) - Add `c` alias for `check` (rust-lang/cargo#6218) - Allow user aliases to override built-in aliases (rust-lang/cargo#6259) - Fix renaming directory project using build scripts with cross-compiling. (rust-lang/cargo#6328) - Fix add_plugin_deps-related tests. (rust-lang/cargo#6327) - Add a glossary. (rust-lang/cargo#6321)
Update cargo, rls 26 commits in b3d0b2e545b61d4cd08096911724b7d49d213f73..5e85ba14aaa20f8133863373404cb0af69eeef2c 2018-11-15 19:13:04 +0000 to 2018-12-02 14:37:25 +0000 - ConflictStoreTrie: Faster filtered search (rust-lang/cargo#6366) - Remove `cmake` as a requirement (rust-lang/cargo#6368) - progress: display "Downloading 1 crate" instead of "Downloading 1 crates" (rust-lang/cargo#6369) - Use expect over unwrap, for panic-in-panic aborts (rust-lang/cargo#6364) - Switch to pretty_env_logger, under --features pretty-env-logger (rust-lang/cargo#6362) - use allow-dirty option in `cargo package` to skip vcs checks (rust-lang/cargo#6280) - remove clones made redundant by Intern PackageId (rust-lang/cargo#6352) - docs: correct profile usage of `cargo test --release` (rust-lang/cargo#6345) - Improve doc for `cargo install` (rust-lang/cargo#6354) - Intern PackageId (rust-lang/cargo#6332) - Clean only release artifacts if --release option is set (rust-lang/cargo#6349) - remove clones made redundant by Intern SourceId (rust-lang/cargo#6347) - Intern SourceId (rust-lang/cargo#6342) - Tweak Layout to allow for non json file targets with internal "." (rust-lang/cargo#6255) - Correct Target Directory command-line option (rust-lang/cargo#6343) - Persistent data structures by im-rs (rust-lang/cargo#6336) - Move command prelude into main library (rust-lang/cargo#6335) - Distinguish custom build invocations (rust-lang/cargo#6331) - Allow crate_type=bin examples to run (rust-lang/cargo#6330) - Make verify-project honour unstable features (rust-lang/cargo#6326) - Make autodiscovery disable inferred targets (rust-lang/cargo#6329) - Add `c` alias for `check` (rust-lang/cargo#6218) - Allow user aliases to override built-in aliases (rust-lang/cargo#6259) - Fix renaming directory project using build scripts with cross-compiling. (rust-lang/cargo#6328) - Fix add_plugin_deps-related tests. (rust-lang/cargo#6327) - Add a glossary. (rust-lang/cargo#6321)
There has been a long standing "TODO: We should switch to persistent hash maps if we can" in the resolver. This PR introduces a dependency on im-rs to provide persistent hash maps. It then uses
im_rc::OrdSetto store the priority list of dependencies, instead ofstd::BinaryHeap, as cloning theHeapwas one of the most expensive things we did. In Big O terms these changes are very big wins, in practice the improvement is small. This is do to a number of factors like,std::collectionsare very fast, N is usually only in the hundreds, we only clone when the borrow checker insists on it, and we useRCand other tricks to avoid cloning.I would advocate that we accept this PR, as:
RCshould be an afterthought not absolutely required before merge. (cc Tracking issue for RFC 1977: public & private dependencies, as it relates to the resolver #6129 witch is stuck on this kind of per tick performance problem as well as other things)../x.py buildenough to trigger an investigation).~5%after, it would be nice to consider if each one is worth the code complexity. (I can list some of the ones I have tried to remove but are still small wins, if that would help the discussion).Overall, I think it is worth doing now, but can see that if is not a slam dunk.