-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: add rustc-hash feature #42
feat: add rustc-hash feature #42
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
[features] | ||
default = [] | ||
|
||
# Switch the hashmaps used in rustdoc-types to the FxHashMap from rustc-hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-ec2
crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`
Changes in preparation of rust-lang/rust#131936 (must be merged after):
rustc-hash
dependency and feature.update.sh
script accordingly.Closes #35