feat(rustdoc-json-types): introduce rustc-hash feature#131936
Merged
bors merged 1 commit intorust-lang:masterfrom Oct 20, 2024
Merged
feat(rustdoc-json-types): introduce rustc-hash feature#131936bors merged 1 commit intorust-lang:masterfrom
bors merged 1 commit intorust-lang:masterfrom
Conversation
This allows the public `rustdoc-types` crate to expose this feature easily and allows consumers of the crate to get the performance advantages from doing so. The reasoning for this was discussed on [Zulip][1] Changes: - Make `rustc-hash` optional but default to including it - Rename all occurrences of `FxHashMap` to `HashMap`. - Feature gate the import and rename the imported `FxHashMap` to `HashMap` - Introduce a type alias `FxHashMap` which resolves to the currently used `HashMap` (`rustc_hash::FxHashMap` or `std::collections::HashMap`) for use in `src/librustdoc`. [1]: https://rust-lang.zulipchat.com/#narrow/channel/266220-t-rustdoc/topic/rustc-hash.20and.20performance.20of.20rustdoc-types
Collaborator
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aDotInTheVoid (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Collaborator
|
rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing |
jalil-salame
added a commit
to jalil-salame/rustdoc-types
that referenced
this pull request
Oct 19, 2024
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
Member
|
Thanks for working on this! @bors r+ rollup |
Collaborator
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 19, 2024
…iaskrgr Rollup of 12 pull requests Successful merges: - rust-lang#116863 (warn less about non-exhaustive in ffi) - rust-lang#127675 (Remove invalid help diagnostics for const pointer) - rust-lang#131772 (Remove `const_refs_to_static` TODO in proc_macro) - rust-lang#131789 (Make sure that outer opaques capture inner opaques's lifetimes even with precise capturing syntax) - rust-lang#131795 (Stop inverting expectation in normalization errors) - rust-lang#131920 (Add codegen test for branchy bool match) - rust-lang#131921 (replace STATX_ALL with (STATX_BASIC_STATS | STATX_BTIME) as former is deprecated) - rust-lang#131925 (Warn on redundant `--cfg` directive when revisions are used) - rust-lang#131931 (Remove unnecessary constness from `lower_generic_args_of_path`) - rust-lang#131932 (use tracked_path in rustc_fluent_macro) - rust-lang#131936 (feat(rustdoc-json-types): introduce rustc-hash feature) - rust-lang#131939 (Get rid of `OnlySelfBounds`) Failed merges: - rust-lang#131181 (Compiletest: Custom differ) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 20, 2024
Rollup merge of rust-lang#131936 - jalil-salame:rustdoc-types-rustc-hash, r=aDotInTheVoid feat(rustdoc-json-types): introduce rustc-hash feature This allows the public `rustdoc-types` crate to expose this feature easily and allows consumers of the crate to get the performance advantages from doing so. The reasoning for this was discussed on [Zulip][1] Changes: - Make `rustc-hash` optional but default to including it - Rename all occurrences of `FxHashMap` to `HashMap`. - Feature gate the import and rename the imported `FxHashMap` to `HashMap` - Introduce a type alias `FxHashMap` which resolves to the currently used `HashMap` (`rustc_hash::FxHashMap` or `std::collections::HashMap`) for use in `src/librustdoc`. [1]: https://rust-lang.zulipchat.com/#narrow/channel/266220-t-rustdoc/topic/rustc-hash.20and.20performance.20of.20rustdoc-types **extra context from the zulip thread:** - `@obi1kenobi` requested benchmarks of the switch to `rustc-hash` - I benchmarked switching `rustdoc-types` to `rustc-hash` which yielded a ~300ms improvement to `cargo-semver-checks`'s index building step (this step is done twice so the improvements are ~150ms per index). - The benchmarks were presented in Zulip and people were in favor of introducing `rustc-hash` to the public `rustdoc-types` crate. - There were differing opinions on how to introduce the dependency: 1. "Hard" dependency: remove use of `std::collections::HashMap` in favor of `FxHashMap`. 2. "Soft" dependency: make optional and introduce a feature then enable/disable it by default (this PR). 3. ~~Make `rustdoc-types` generic and expose the `RandomState`~~ (a lot of work & complexity for little gain over a feature gate). `@obi1kenobi` and I prefer the feature gate so that is what I am adding here. My reasons for the preference are: - `cargo-semver-checks` is especially perf sensitive, we don't expect people to care about ~150ms extra time when reading in a 500MB file (the size of the sample we used for benchmarking). - Keeping `rustdoc-types` lean by having its only direct dependency be `serde` is nice for the general consumer of the crate. - `rustc-hash` is not HashDOS resistant (but it is questionable whether `rustdoc-types` would be used on adversarial inputs). r? `@aDotInTheVoid`
jalil-salame
added a commit
to jalil-salame/rustdoc-types
that referenced
this pull request
Oct 20, 2024
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
jalil-salame
added a commit
to jalil-salame/rustdoc-types
that referenced
this pull request
Oct 20, 2024
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
aDotInTheVoid
added a commit
to rust-lang/rustdoc-types
that referenced
this pull request
Oct 20, 2024
* feat: add rustc-hash feature 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 * chore: run ./update.sh * feat(ci): also test with the `rustc-hash` feature * README reword --------- Co-authored-by: Alona Enraght-Moony <code@alona.page>
jalil-salame
added a commit
to jalil-salame/rust
that referenced
this pull request
Oct 20, 2024
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
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 20, 2024
…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`
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 21, 2024
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`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This allows the public
rustdoc-typescrate to expose this feature easily and allows consumers of the crate to get the performance advantages from doing so.The reasoning for this was discussed on Zulip
Changes:
rustc-hashoptional but default to including itFxHashMaptoHashMap.FxHashMaptoHashMapFxHashMapwhich resolves to the currently usedHashMap(rustc_hash::FxHashMaporstd::collections::HashMap) for use insrc/librustdoc.extra context from the zulip thread:
rustc-hashrustdoc-typestorustc-hashwhich yielded a ~300ms improvement tocargo-semver-checks's index building step (this step is done twice so the improvements are ~150ms per index).rustc-hashto the publicrustdoc-typescrate.std::collections::HashMapin favor ofFxHashMap.Make(a lot of work & complexity for little gain over a feature gate).rustdoc-typesgeneric and expose theRandomState@obi1kenobi and I prefer the feature gate so that is what I am adding here.
My reasons for the preference are:
cargo-semver-checksis especially perf sensitive, we don't expect people to care about ~150ms extra time when reading in a 500MB file (the size of the sample we used for benchmarking).rustdoc-typeslean by having its only direct dependency beserdeis nice for the general consumer of the crate.rustc-hashis not HashDOS resistant (but it is questionable whetherrustdoc-typeswould be used on adversarial inputs).r? @aDotInTheVoid