feat: add rustc-hash feature#42
Conversation
aDotInTheVoid
left a comment
There was a problem hiding this comment.
Thanks for working on this. The approach looks right.
Could you:
- Update the code with the script (by pointing at your repo for now) so we can check it looks correct
- Update the CI to test with the new feature included.
| @@ -9,9 +9,7 @@ repo="rust" | |||
| branch="master" | |||
There was a problem hiding this comment.
You can use these user/repo/branch variables to point the update script at your fork/branch, so that you can include the new code in here, so it's easier to see the effect of the change.
| [features] | ||
| default = [] | ||
|
|
||
| # Switch the hashmaps used in rustdoc-types to the FxHashMap from rustc-hash. |
There was a problem hiding this comment.
I think these documentation should go in either the README.md, or the crate-level doc comment (or both)
There was a problem hiding this comment.
I think all three! Because the docs.rs feature flags tab is a great place and should be used more c:
E.g. clap's
There was a problem hiding this comment.
As far as I can tell, I'd need to do another PR to rust-lang/rust to modify the crate level doc comment.
In the mean time I modified the README to add more context
There was a problem hiding this comment.
Yeah. it'll need to be in rust-lang/rust, but I don't think it's worth blocking this PR on that being done.
fc77030 to
e7976ec
Compare
484cd78 to
44e2899
Compare
aDotInTheVoid
left a comment
There was a problem hiding this comment.
Almost there, just needs tweaks to the README wording to be more useful to users.
| [features] | ||
| default = [] | ||
|
|
||
| # Switch the hashmaps used in rustdoc-types to the FxHashMap from rustc-hash. |
There was a problem hiding this comment.
Yeah. it'll need to be in rust-lang/rust, but I don't think it's worth blocking this PR on that being done.
README.md
Outdated
| `rustc-hash::FxHashMap` which improves performance when reading big JSON files | ||
| (like `aws_sdk_rs`'s). | ||
|
|
||
| We have tested this on `cargo-semver-checks` using `aws_sdk_ec2`'s JSON and |
There was a problem hiding this comment.
I don't like the use of "We" here. I think it's important to have a clear line between what's maintained by the rust project, and what isn't.
Additionally I don't think the numbers presented here are particularly useful to people who don't know/care about csc internals. I think just a percentage improvement would much more useful.
In testing on [cargo-semver-checks](some link), this provided a whatever% perfomance improvement on the
aws-sdk-ec2crate.
There was a problem hiding this comment.
Reworded, I hope this is better?
Changes in preparation of [rust-lang/rust#131936][1]: - Introduce `rustc-hash` dependency and feature. - Modify the `update.sh` script accordingly. [1]: rust-lang/rust#131936
44e2899 to
28b4f6e
Compare
The `rustc-hash` feature is publicly exposed by the `rustdoc-types`. It is already documented in that crate's README and Cargo.toml, but we might as well add some information to the crate docs themselves c: Follow up to: - rust-lang#131936 - [rust-lang/rustdoc-types#42][1] [1]: rust-lang/rustdoc-types#42
…t-feature, r=aDotInTheVoid fix(rustdoc-json-types): document rustc-hash feature The `rustc-hash` feature is publicly exposed by the `rustdoc-types`. It is already documented in that crate's README and Cargo.toml, but we might as well add some information to the crate docs themselves c: Follow up to: - rust-lang#131936 - [rust-lang/rustdoc-types#42][1] [1]: rust-lang/rustdoc-types#42 r? `@aDotInTheVoid`
Rollup merge of rust-lang#131973 - jalil-salame:rustdoc-types-document-feature, r=aDotInTheVoid fix(rustdoc-json-types): document rustc-hash feature The `rustc-hash` feature is publicly exposed by the `rustdoc-types`. It is already documented in that crate's README and Cargo.toml, but we might as well add some information to the crate docs themselves c: Follow up to: - rust-lang#131936 - [rust-lang/rustdoc-types#42][1] [1]: rust-lang/rustdoc-types#42 r? `@aDotInTheVoid`
| rustc-hash = {version="2", optional=true} | ||
|
|
||
| [features] | ||
| default = [] |
There was a problem hiding this comment.
@jalil-salame Do you remember why this was added? It seems unused, but I want to make sure I'm not removing something needed.
There was a problem hiding this comment.
The default feature? I probably added it to be explicit about not having anything turned on by default
Changes in preparation of rust-lang/rust#131936 (must be merged after):
rustc-hashdependency and feature.update.shscript accordingly.Closes #35