Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(rustdoc-json-types): introduce rustc-hash feature #131936

Merged
merged 1 commit into from
Oct 20, 2024
Merged

feat(rustdoc-json-types): introduce rustc-hash feature #131936

merged 1 commit into from
Oct 20, 2024

Conversation

jalil-salame
Copy link
Contributor

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

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.

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
feat(rustdoc-json-types): introduce rustc-hash feature
d1fa49b 
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
@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2024

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2024

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

jalil-salame added a commit to jalil-salame/rustdoc-types that referenced this pull request Oct 19, 2024
@jalil-salame
feat: add rustc-hash feature
fc77030 
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 jalil-salame mentioned this pull request Oct 19, 2024
Merged
@aDotInTheVoid
Copy link
Member

Thanks for working on this!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 19, 2024

📌 Commit d1fa49b has been approved by aDotInTheVoid

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 19, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 19, 2024
@bors
Auto merge of rust-lang#131948 - matthiaskrgr:rollup-c9rvzu6, r=matth…
b596184 
…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
@bors bors merged commit ab7e0d0 into rust-lang:master Oct 20, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2024
@rust-timer
Unrolled build for rust-lang#131936
7fdb5dc 
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
@jalil-salame
feat: add rustc-hash feature
360ff0c 
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 jalil-salame deleted the rustdoc-types-rustc-hash branch October 20, 2024 14:23
jalil-salame added a commit to jalil-salame/rustdoc-types that referenced this pull request Oct 20, 2024
@jalil-salame
feat: add rustc-hash feature
cfdba89 
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
@jalil-salame @aDotInTheVoid
feat: add rustc-hash feature (#42)
898a57d 
* 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
@jalil-salame
fix(rustdoc-json-types): document rustc-hash feature
880d565 
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
@jalil-salame jalil-salame mentioned this pull request Oct 20, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 20, 2024
@matthiaskrgr
Rollup merge of rust-lang#131973 - jalil-salame:rustdoc-types-documen…
c2ed685 
…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
@rust-timer
Unrolled build for rust-lang#131973
060e346 
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aDotInTheVoid aDotInTheVoid

Labels
A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jalil-salame @rustbot @aDotInTheVoid @bors