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.

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

Merged
merged 4 commits into from
Oct 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,13 @@ env:
jobs:
cargo-test:
runs-on: ubuntu-latest
strategy:
matrix:
features:
- default
- rustc-hash
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
- uses: Swatinem/rust-cache@v2
- run: cargo test
- run: cargo test --no-default-features --features '${{ matrix.features }}'
2 changes: 1 addition & 1 deletion COMMIT.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2e6f3bd1d32455e535de1d9ee154253c333aec73
d1fa49b2e66c343210c413b68ed57f150b7b89d8
10 changes: 10 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ repository = "https://github.com/rust-lang/rustdoc-types"

[dependencies]
serde = {version="1", features=["derive"]}
rustc-hash = {version="2", optional=true}

[features]
default = []

# Switch the hashmaps used in rustdoc-types to the FxHashMap from rustc-hash.
Copy link
Member

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)

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

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.

#
# This might improve performace if your are reading the rustdoc JSON from large
# crates like aws_sdk_ec2
rustc-hash = ["dep:rustc-hash"]

[dev-dependencies]
bincode = "1.3.3"
Expand Down
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@ let krate: rustdoc_types::Crate = serde_json::from_str(&json_string)?;
println!("the index has {} items", krate.index.len());
```

For performance sensitive crates, consider turning on the `rustc-hash`
feature. This switches all data structures from `std::collections::HashMap` to
`rustc-hash::FxHashMap` which improves performance when reading big JSON files
(like `aws_sdk_rs`'s).

`cargo-semver-checks` benchmarked this change with `aws_sdk_ec2`'s JSON and
[observed a -3% improvement to the runtime][csc benchmarks]. The performance
here depends on how much time you spend querying the `HashMap`s, so as always,
measure first.

[csc benchmarks]: https://rust-lang.zulipchat.com/#narrow/channel/266220-t-rustdoc/topic/rustc-hash.20and.20performance.20of.20rustdoc-types/near/474855731

## Contributing

This repo is a reexport of
Expand Down
6 changes: 5 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,15 @@
//! These types are the public API exposed through the `--output-format json` flag. The [`Crate`]
//! struct is the root of the JSON blob and all other items are contained within.

#[cfg(not(feature = "rustc-hash"))]
use std::collections::HashMap;
use std::path::PathBuf;

use std::collections::HashMap;
#[cfg(feature = "rustc-hash")]
use rustc_hash::FxHashMap as HashMap;
use serde::{Deserialize, Serialize};


/// The version of JSON output that this crate represents.
///
/// This integer is incremented with every breaking change to the API,
Expand Down
4 changes: 1 addition & 3 deletions update.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ repo="rust"
branch="master"
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


curl -# https://raw.githubusercontent.com/${user}/${repo}/${branch}/src/rustdoc-json-types/lib.rs \
| sed 's/rustc_hash::/std::collections::/g' \
| sed 's/FxHashMap/HashMap/g' \
| sed 's/^pub use /use /' \
| sed '/^pub type FxHashMap.*$/d' \
> src/lib.rs

curl -# https://raw.githubusercontent.com/${user}/${repo}/${branch}/src/rustdoc-json-types/tests.rs > src/tests.rs
Expand Down