rustdoc-json: switch from HashMap to FxHashMap to fix non-determinism by onur-ozkan · Pull Request #108626 · rust-lang/rust

onur-ozkan · 2023-03-01T21:57:37Z

Using HashMap in rustdoc_json_types::Crate were causing creating randomly ordered objects in the json doc files. Which might cause problems to people who are doing comparison on those files specially in CI pipelines. See #103785 (comment)

This PR fixes that issue and extends the coverage of tests/run-make/rustdoc-verify-output-files testing ability.

rustbot · 2023-03-01T21:57:45Z

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-03-01T21:57:48Z

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

src/rustdoc-json-types/lib.rs

aDotInTheVoid · 2023-03-02T12:38:33Z

r? @aDotInTheVoid

I'm a bit nervous about the performance impact of this, as it changes the core data structure of rustdoc-json. Unfortunately we don't have any automated way to see if this actually matters (cc rust-lang/rustc-perf#1512)

Could you use a FxHashMap instead, as that should get us deterministic results, even if the IDs arn't sorted, but I don't think that matters since users shouldn't be relying on content (and therefor ordering) of IDs.

jyn514 · 2023-03-02T13:40:09Z

Could you use a FxHashMap instead, as that should get us deterministic results

I don't think this is true? FxHashMap is just changing the default hasher for performance, it doesn't affect the order. I vaguely recall there's a StableHasher type of something but it's not the same as the FxHasher.

aDotInTheVoid · 2023-03-02T13:56:02Z

FxHashMap is just changing the default hasher for performance, it doesn't affect the order.

The relevant difference is that two FxHashMaps with the same order of inserts will always iterate in the same order, as they don't randomize the hash seed (unlike std::collections::HashMap). StableHasher provides additional garenttes about the archetecure we are compilling from, that I don't think we need to make

obi1kenobi · 2023-03-02T15:38:22Z

Unfortunately we don't have any automated way to see if this actually matters

As part of my performance work on cargo-semver-checks that led to my "Speeding up Rust semver-checking by over 2000x" blog post, I did some benchmarking of HashMap vs BTreeMap as secondary indexes over rustdoc (code here). To a rounding error, the maps there are of the same size as the rustdoc index and paths maps, and one of them contains Id as part of the key. I also didn't do a ton of quiescing my system (disable hyperthreading and turbo boost, fix fan speed, do thermal warmup, etc.). So treat my findings as anecdotal + directional rather than authoritative + exact. They are not a replacement for proper benchmark tests of rustdoc itself.

I found that using HashMap was consistently noticeably faster than BTreeMap over a gigantic crate (aws-sdk-ec2, 240k items), by about 15-20% i.e. ~1-2s per run.

This doesn't say that we shouldn't replace HashMap with BTreeMap. It only says that there's good reason for performance concern, and we should benchmark more thoroughly before making a substantive change like that.

If anyone would like to repeat my experiments, I'd be happy to help you get set up. And if cargo-semver-checks would be useful as a part of a perf benchmark suite, I'd be happy to assist in that process too.

onur-ozkan · 2023-03-02T18:42:41Z

Could you use a FxHashMap instead, as that should get us deterministic results, even if the IDs arn't sorted, but I don't think that matters since users shouldn't be relying on content (and therefor ordering) of IDs.

I didn't want to involve rustc_private into rustdoc-json-types for this. I can do it if you want so.

For the performance concerns, here is a basic demonstration from my desktop machine.

using `BTreeMap`

Build completed successfully in 0:00:15

 Performance counter stats for '../../rust/x doc --stage 1 std --json':

         15,873.45 msec task-clock:u              #    1.000 CPUs utilized
                 0      context-switches:u        #    0.000 K/sec
                 0      cpu-migrations:u          #    0.000 K/sec
           419,374      page-faults:u             #    0.026 M/sec
    62,660,633,799      cycles:u                  #    3.948 GHz                      (83.38%)
       512,794,289      stalled-cycles-frontend:u #    0.82% frontend cycles idle     (83.39%)
        80,806,547      stalled-cycles-backend:u  #    0.13% backend cycles idle      (83.38%)
    82,024,064,071      instructions:u            #    1.31  insn per cycle
                                                  #    0.01  stalled cycles per insn  (83.36%)
    16,035,947,363      branches:u                # 1010.237 M/sec                    (83.38%)
       201,578,639      branch-misses:u           #    1.26% of all branches          (83.32%)

      15.867989639 seconds time elapsed

      14.909949000 seconds user
       0.826553000 seconds sys

using `FxHashMap`

Build completed successfully in 0:00:15

 Performance counter stats for '../../rust/x doc --stage 1 std --json':

         15,548.93 msec task-clock:u              #    1.000 CPUs utilized
                 0      context-switches:u        #    0.000 K/sec
                 0      cpu-migrations:u          #    0.000 K/sec
           423,520      page-faults:u             #    0.027 M/sec
    61,458,365,022      cycles:u                  #    3.953 GHz                      (83.37%)
       505,423,215      stalled-cycles-frontend:u #    0.82% frontend cycles idle     (83.37%)
       710,815,053      stalled-cycles-backend:u  #    1.16% backend cycles idle      (83.40%)
    81,826,900,373      instructions:u            #    1.33  insn per cycle
                                                  #    0.01  stalled cycles per insn  (83.31%)
    15,968,492,682      branches:u                # 1026.984 M/sec                    (83.35%)
       201,022,039      branch-misses:u           #    1.26% of all branches          (83.40%)

      15.542970326 seconds time elapsed

      14.672585000 seconds user
       0.740135000 seconds sys

HashMap benchmark is just for the comparison, I think we should avoid using it. It produces the problem mentioned in the PR description.

using `HashMap`

Build completed successfully in 0:00:15

 Performance counter stats for '../../rust/x doc --stage 1 std --json':

         15,617.23 msec task-clock:u              #    1.000 CPUs utilized
                 0      context-switches:u        #    0.000 K/sec
                 0      cpu-migrations:u          #    0.000 K/sec
           425,083      page-faults:u             #    0.027 M/sec
    61,592,835,803      cycles:u                  #    3.944 GHz                      (83.39%)
       490,297,782      stalled-cycles-frontend:u #    0.80% frontend cycles idle     (83.37%)
       727,341,887      stalled-cycles-backend:u  #    1.18% backend cycles idle      (83.38%)
    81,851,581,044      instructions:u            #    1.33  insn per cycle
                                                  #    0.01  stalled cycles per insn  (83.37%)
    15,975,646,762      branches:u                # 1022.950 M/sec                    (83.38%)
       202,179,644      branch-misses:u           #    1.27% of all branches          (83.31%)

      15.609654475 seconds time elapsed

      14.611848000 seconds user
       0.869682000 seconds sys

Those runs were on the precompiled bootstrap/compiler/library. All went directly to generating the json files.

Machine specs
CPU: AMD Ryzen 9 5900X (24) @ 4.200GHz
MEMORY: 32 GB DDR4 2666 MHz
OS: Void Linux @ Kernel: 6.1.14_1

Regarding to the results, FxHashMap seems fixing the problem with less performance impact compare to BTreeMap.

aDotInTheVoid · 2023-03-02T20:25:42Z

I didn't want to involve rustc_private into rustdoc-json-types for this.

It's fine, as this is only ment for use in the rust-lang/rust repo. The reexport I maintain for crates.io already handles this. While it's important that this library can be used on crates.io and stable, adding rustc_data_structures as a dep won't change that

onur-ozkan · 2023-03-02T22:25:27Z

I didn't want to involve rustc_private into rustdoc-json-types for this.

It's fine, as this is only ment for use in the rust-lang/rust repo. The reexport I maintain for crates.io already handles this. While it's important that this library can be used on crates.io and stable, adding rustc_data_structures as a dep won't change that

rustc_data_structures will fail jsondoclint because of the rustc_private feature. Is it okay if I use rustc_hash instead of rustc_data_structures?

I will push it using rustc_hash, if it's not okay, you can share your concerns with on the review

rustbot · 2023-03-02T22:45:49Z

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

aDotInTheVoid · 2023-03-04T11:56:38Z

Is it okay if I use rustc_hash instead of rustc_data_structures?

I'm not sure what the policy on this is. I've asked on zulip

aDotInTheVoid · 2023-03-04T14:48:35Z

Turns out it's fine. Thanks for fixing this.

@bors r+

bors · 2023-03-04T14:48:37Z

📌 Commit df061c8 has been approved by aDotInTheVoid

It is now in the queue for this repository.

src/rustdoc-json-types/lib.rs

Signed-off-by: ozkanonur <work@onurozkan.dev>

aDotInTheVoid · 2023-03-04T21:42:32Z

r=me when CI's green

onur-ozkan · 2023-03-04T21:51:46Z

r? @aDotInTheVoid

rustbot · 2023-03-04T21:51:47Z

Could not assign reviewer from: aDotInTheVoid.
User(s) aDotInTheVoid are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

aDotInTheVoid · 2023-03-04T22:43:05Z

@bors r+

bors · 2023-03-04T22:43:08Z

📌 Commit 52c71e6 has been approved by aDotInTheVoid

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#106440 (Ignore files in .gitignore in tidy) - rust-lang#108613 (Remove `llvm.skip-rebuild` option) - rust-lang#108616 (Sync codegen defaults with compiler defaults and add a ping message so they stay in sync) - rust-lang#108618 (Rename `src/etc/vscode_settings.json` to `rust_analyzer_settings.json`) - rust-lang#108626 (rustdoc-json: switch from HashMap to FxHashMap to fix non-determinism) - rust-lang#108744 (Don't ICE when encountering bound var in builtin copy/clone bounds) - rust-lang#108749 (Clean up rustdoc-js tester.js file) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

`rustc_data_structures` rust-lang/rust#108626

The original motivation was me trying to remove the `#![allow(rustc::default_hash_types)]`, as after rust-lang#108626, we should be using `FxHashMap` here. I then realized I should also be able to remove the `.into_iter().collect()`, as we no longer need to convert from `FxHashMap` to `std::HashMap`. However, this didn't work, and I got the following error ``` error[E0308]: mismatched types --> src/librustdoc/json/mod.rs:235:13 | 235 | index, | ^^^^^ expected `rustc_hash::FxHasher`, found `FxHasher` | = note: `FxHasher` and `rustc_hash::FxHasher` have similar names, but are actually distinct types note: `FxHasher` is defined in crate `rustc_hash` --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 note: `rustc_hash::FxHasher` is defined in crate `rustc_hash` --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 | 60 | pub struct FxHasher { | ^^^^^^^^^^^^^^^^^^^ = note: perhaps two different versions of crate `rustc_hash` are being used? ``` This is because `librustdoc` got it's `FxHashMap` via the sysroot `rustc-data-strucures`, whereas `rustdoc-json-types` got it via `rustc-hash` on crates.io. To avoid this, `rustdoc-json-types` now uses `#![feature(rustc_private)]` to load the same version as `librustdoc`. However, this needs to be placed behind a feature, as `rustdoc-json-types` is also dependency of `src/tools/jsondocck`, which means need needs to be buildable without nightly features.

Use same `FxHashMap` in `rustdoc-json-types` and `librustdoc`. The original motivation was me trying to remove the `#![allow(rustc::default_hash_types)]`, as after rust-lang#108626, we should be using `FxHashMap` here. I then realized I should also be able to remove the `.into_iter().collect()`, as we no longer need to convert from `FxHashMap` to `std::HashMap`. However, this didn't work, and I got the following error ``` error[E0308]: mismatched types --> src/librustdoc/json/mod.rs:235:13 | 235 | index, | ^^^^^ expected `rustc_hash::FxHasher`, found `FxHasher` | = note: `FxHasher` and `rustc_hash::FxHasher` have similar names, but are actually distinct types note: `FxHasher` is defined in crate `rustc_hash` --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 note: `rustc_hash::FxHasher` is defined in crate `rustc_hash` --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 | 60 | pub struct FxHasher { | ^^^^^^^^^^^^^^^^^^^ = note: perhaps two different versions of crate `rustc_hash` are being used? ``` This is because `librustdoc` got it's `FxHashMap` via the sysroot `rustc-data-strucures`, whereas `rustdoc-json-types` got it via `rustc-hash` on crates.io. To avoid this, `rustdoc-json-types` now uses `#![feature(rustc_private)]` to load the same version as `librustdoc`. However, this needs to be placed behind a feature, as `rustdoc-json-types` is also dependency of `src/tools/jsondocck`, which means need needs to be buildable without nightly features. r? `@jyn514`

The original motivation was me trying to remove the `#![allow(rustc::default_hash_types)]`, as after rust-lang#108626, we should be using `FxHashMap` here. I then realized I should also be able to remove the `.into_iter().collect()`, as we no longer need to convert from `FxHashMap` to `std::HashMap`. However, this didn't work, and I got the following error ``` error[E0308]: mismatched types --> src/librustdoc/json/mod.rs:235:13 | 235 | index, | ^^^^^ expected `rustc_hash::FxHasher`, found `FxHasher` | = note: `FxHasher` and `rustc_hash::FxHasher` have similar names, but are actually distinct types note: `FxHasher` is defined in crate `rustc_hash` --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 note: `rustc_hash::FxHasher` is defined in crate `rustc_hash` --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 | 60 | pub struct FxHasher { | ^^^^^^^^^^^^^^^^^^^ = note: perhaps two different versions of crate `rustc_hash` are being used? ``` This is because `librustdoc` got it's `FxHashMap` via the sysroot `rustc-data-strucures`, whereas `rustdoc-json-types` got it via `rustc-hash` on crates.io. To avoid this, `rustdoc-json-types` now uses `#![feature(rustc_private)]` to load the same version as `librustdoc`. However, this needs to be placed behind a feature, as `rustdoc-json-types` is also dependency of `src/tools/jsondocck`, which means need needs to be buildable without nightly features.

Use same `FxHashMap` in `rustdoc-json-types` and `librustdoc`. The original motivation was me trying to remove the `#![allow(rustc::default_hash_types)]`, as after rust-lang#108626, we should be using `FxHashMap` here. I then realized I should also be able to remove the `.into_iter().collect()`, as we no longer need to convert from `FxHashMap` to `std::HashMap`. However, this didn't work, and I got the following error ``` error[E0308]: mismatched types --> src/librustdoc/json/mod.rs:235:13 | 235 | index, | ^^^^^ expected `rustc_hash::FxHasher`, found `FxHasher` | = note: `FxHasher` and `rustc_hash::FxHasher` have similar names, but are actually distinct types note: `FxHasher` is defined in crate `rustc_hash` --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 note: `rustc_hash::FxHasher` is defined in crate `rustc_hash` --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 | 60 | pub struct FxHasher { | ^^^^^^^^^^^^^^^^^^^ = note: perhaps two different versions of crate `rustc_hash` are being used? ``` This is because `librustdoc` got it's `FxHashMap` via the sysroot `rustc-data-strucures`, whereas `rustdoc-json-types` got it via `rustc-hash` on crates.io. To avoid this, `rustdoc-json-types` now uses `#![feature(rustc_private)]` to load the same version as `librustdoc`. However, this needs to be placed behind a feature, as `rustdoc-json-types` is also dependency of `src/tools/jsondocck`, which means need needs to be buildable without nightly features. r? `@jyn514`

Use same `FxHashMap` in `rustdoc-json-types` and `librustdoc`. The original motivation was me trying to remove the `#![allow(rustc::default_hash_types)]`, as after rust-lang#108626, we should be using `FxHashMap` here. I then realized I should also be able to remove the `.into_iter().collect()`, as we no longer need to convert from `FxHashMap` to `std::HashMap`. However, this didn't work, and I got the following error ``` error[E0308]: mismatched types --> src/librustdoc/json/mod.rs:235:13 | 235 | index, | ^^^^^ expected `rustc_hash::FxHasher`, found `FxHasher` | = note: `FxHasher` and `rustc_hash::FxHasher` have similar names, but are actually distinct types note: `FxHasher` is defined in crate `rustc_hash` --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 note: `rustc_hash::FxHasher` is defined in crate `rustc_hash` --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 | 60 | pub struct FxHasher { | ^^^^^^^^^^^^^^^^^^^ = note: perhaps two different versions of crate `rustc_hash` are being used? ``` This is because `librustdoc` got it's `FxHashMap` via the sysroot `rustc-data-strucures`, whereas `rustdoc-json-types` got it via `rustc-hash` on crates.io. To avoid this, `rustdoc-json-types` now uses `#![feature(rustc_private)]` to load the same version as `librustdoc`. However, this needs to be placed behind a feature, as `rustdoc-json-types` is also dependency of `src/tools/jsondocck`, which means need needs to be buildable without nightly features. r? ``@jyn514``

The original motivation was me trying to remove the `#![allow(rustc::default_hash_types)]`, as after rust-lang#108626, we should be using `FxHashMap` here. I then realized I should also be able to remove the `.into_iter().collect()`, as we no longer need to convert from `FxHashMap` to `std::HashMap`. However, this didn't work, and I got the following error ``` error[E0308]: mismatched types --> src/librustdoc/json/mod.rs:235:13 | 235 | index, | ^^^^^ expected `rustc_hash::FxHasher`, found `FxHasher` | = note: `FxHasher` and `rustc_hash::FxHasher` have similar names, but are actually distinct types note: `FxHasher` is defined in crate `rustc_hash` --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 note: `rustc_hash::FxHasher` is defined in crate `rustc_hash` --> /home/alona/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustc-hash-1.1.0/src/lib.rs:60:1 | 60 | pub struct FxHasher { | ^^^^^^^^^^^^^^^^^^^ = note: perhaps two different versions of crate `rustc_hash` are being used? ``` This is because `librustdoc` got it's `FxHashMap` via the sysroot `rustc-data-strucures`, whereas `rustdoc-json-types` got it via `rustc-hash` on crates.io. To avoid this, `rustdoc-json-types` now uses `#![feature(rustc_private)]` to load the same version as `librustdoc`. However, this needs to be placed behind a feature, as `rustdoc-json-types` is also dependency of `src/tools/jsondocck`, which means need needs to be buildable without nightly features.

rustbot assigned jsha Mar 1, 2023

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 Mar 1, 2023

onur-ozkan force-pushed the consistent-json-docs branch 2 times, most recently from e921fe9 to 02b32e6 Compare March 1, 2023 22:17

jyn514 reviewed Mar 1, 2023

View reviewed changes

src/rustdoc-json-types/lib.rs Outdated Show resolved Hide resolved

onur-ozkan force-pushed the consistent-json-docs branch from 3f26808 to 098cf6d Compare March 1, 2023 23:14

rustbot assigned aDotInTheVoid and unassigned jsha Mar 2, 2023

aDotInTheVoid changed the title ~~fix inconsistent json outputs from rustdoc~~ rustdoc-json: Don't have non-deterministic output due to HashMap Mar 2, 2023

aDotInTheVoid mentioned this pull request Mar 2, 2023

Add rustdoc-json perf check rust-lang/rustc-perf#1512

Closed

onur-ozkan force-pushed the consistent-json-docs branch 2 times, most recently from 278535c to df061c8 Compare March 2, 2023 22:49

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 Mar 4, 2023

aDotInTheVoid changed the title ~~rustdoc-json: Don't have non-deterministic output due to HashMap~~ rustdoc-json: switch from HashMap to FxHashMap to fix non-determinism Mar 4, 2023

onur-ozkan commented Mar 4, 2023

View reviewed changes

src/rustdoc-json-types/lib.rs Outdated Show resolved Hide resolved

fix inconsistent json outputs from rustdoc

52c71e6

Signed-off-by: ozkanonur <work@onurozkan.dev>

onur-ozkan force-pushed the consistent-json-docs branch from 3f8d3ab to 52c71e6 Compare March 4, 2023 21:09

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2023

matthiaskrgr mentioned this pull request Mar 5, 2023

Rollup of 7 pull requests #108771

Merged

bors merged commit 03c1e4d into rust-lang:master Mar 5, 2023

rustbot added this to the 1.70.0 milestone Mar 5, 2023

aDotInTheVoid added a commit to rust-lang/rustdoc-types that referenced this pull request Mar 6, 2023

update.sh: Expect FxHashMap to come from rustc_hash, not

ed26026

`rustc_data_structures` rust-lang/rust#108626

aDotInTheVoid mentioned this pull request Apr 7, 2023

Use same FxHashMap in rustdoc-json-types and librustdoc. #110051

Closed

Uh oh!

Comments

Conversation

onur-ozkan commented Mar 1, 2023

Uh oh!

rustbot commented Mar 1, 2023

Uh oh!

rustbot commented Mar 1, 2023

Uh oh!

Uh oh!

aDotInTheVoid commented Mar 2, 2023

Uh oh!

jyn514 commented Mar 2, 2023

Uh oh!

aDotInTheVoid commented Mar 2, 2023

Uh oh!

obi1kenobi commented Mar 2, 2023

Uh oh!

onur-ozkan commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aDotInTheVoid commented Mar 2, 2023

Uh oh!

onur-ozkan commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 2, 2023

Uh oh!

aDotInTheVoid commented Mar 4, 2023

Uh oh!

aDotInTheVoid commented Mar 4, 2023

Uh oh!

bors commented Mar 4, 2023

Uh oh!

Uh oh!

aDotInTheVoid commented Mar 4, 2023

Uh oh!

onur-ozkan commented Mar 4, 2023

Uh oh!

rustbot commented Mar 4, 2023

Uh oh!

aDotInTheVoid commented Mar 4, 2023

Uh oh!

bors commented Mar 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

onur-ozkan commented Mar 2, 2023 •

edited

Loading

onur-ozkan commented Mar 2, 2023 •

edited

Loading