[rustdoc] Use Map instead of Object for source files and search index #118910

GuillaumeGomez · 2023-12-13T11:16:38Z

It's cleaner and is also easier to manipulate Map rather than Object types.

rustbot · 2023-12-13T11:16:48Z

Some changes occurred in HTML/CSS/JS.

notriddle · 2023-12-13T17:33:22Z

src/librustdoc/html/render/search_index.rs

@@ -488,7 +488,7 @@ pub(crate) fn build_index<'tcx>(

    // Collect the index into a string
    format!(
-        r#""{}":{}"#,
+        r#"["{}",{}]"#,


I would prefer not to make changes that add more bytes to these files. Instead, could we keep the object representation in JSON and add a function to convert to a map at the end?

function rustdocObjectToMap(obj) { return new Map(Object.entries(obj)); }

Hum, that kinda kills the original point. Do you mind running a perf check on this already before I make the change please? Like that we'll be able to see if how we transform into a Map changes anything.

Yeah, that makes sense.

Oh I meant on your JS perf check (I don't think there will be any change on the rust side). I couldn't find it on your profile. ^^'

https://gitlab.com/notriddle/rustdoc-js-profile

I was looking on github so now I understand why I didn't find it... Thanks!

There doesn’t seem to be any significant difference in doc bytes.

I’ll try running the performance test on it as soon as the merge conflicts are resolved.

notriddle · 2023-12-13T19:02:39Z

@bors try @rust-timer queue

bors · 2023-12-13T19:03:50Z

⌛ Trying commit 7502c44 with merge f359997...

…try> [rustdoc] Use Map instead of Object for source files and search index It's cleaner and is also easier to manipulate `Map` rather than `Object` types. r? `@notriddle`

bors · 2023-12-13T20:32:26Z

☀️ Try build successful - checks-actions
Build commit: f359997 (f359997bbe4cfd8cc0fdd406ce2e41f755e7e03a)

rust-timer · 2023-12-14T00:16:52Z

Finished benchmarking commit (f359997): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.5%, 2.0%]	3
Regressions ❌ (secondary)	3.5%	[3.3%, 3.6%]	2
Improvements ✅ (primary)	-1.8%	[-1.8%, -1.8%]	1
Improvements ✅ (secondary)	-3.4%	[-4.7%, -2.2%]	2
All ❌✅ (primary)	0.3%	[-1.8%, 2.0%]	4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.5%, 0.6%]	3

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 673.56s -> 672.051s (-0.22%)
Artifact size: 312.39 MiB -> 312.40 MiB (0.00%)

GuillaumeGomez · 2023-12-14T12:38:48Z

Fixed the merge conflict and thanks in advance for the JS checks. :)

notriddle · 2023-12-14T15:48:26Z

Okay, here's the profile.

http://notriddle.com/rustdoc-html-demo-8/profile-map/index.html

There's also no noticeable change, so let's go with it.

@bors r+ rollup

bors · 2023-12-14T15:48:29Z

📌 Commit fc72216 has been approved by notriddle

It is now in the queue for this repository.

GuillaumeGomez · 2023-12-14T15:50:32Z

It tends to be a little bit faster to build actually. Interesting. I wonder if this difference comes from the switch to an array in the JSON or that the Map is just that much faster.

…=notriddle [rustdoc] Use Map instead of Object for source files and search index It's cleaner and is also easier to manipulate `Map` rather than `Object` types. r? `@notriddle`

notriddle · 2023-12-14T15:57:14Z

Tiny differences are pretty likely to not be real. This is running on my desktop, and isn't a very tightly controlled environment.

I'm just happy not to see any order of magnitude blow-ups.

GuillaumeGomez · 2023-12-14T15:58:07Z

One can hope. ;)

…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#118770 (Fix cases where std accidentally relied on inline(never)) - rust-lang#118910 ([rustdoc] Use Map instead of Object for source files and search index) - rust-lang#118914 (Unconditionally register alias-relate in projection goal) - rust-lang#118935 (interpret: extend comment on the inhabitedness check in downcast) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#118910 ([rustdoc] Use Map instead of Object for source files and search index) - rust-lang#118914 (Unconditionally register alias-relate in projection goal) - rust-lang#118935 (interpret: extend comment on the inhabitedness check in downcast) - rust-lang#118945 (rustc_codegen_ssa: Remove trailing spaces in Display impl for CguReuse) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#118910 - GuillaumeGomez:js-object-to-map, r=notriddle [rustdoc] Use Map instead of Object for source files and search index It's cleaner and is also easier to manipulate `Map` rather than `Object` types. r? `@notriddle`

notriddle · 2023-12-15T02:30:44Z

src/librustdoc/html/render/write_shared.rs

@@ -296,9 +297,9 @@ pub(super) fn write_shared(
                    .replace("\\\"", "\\\\\"")
            ));
            all_sources.sort();
-            let mut v = String::from("var srcIndex = JSON.parse('{\\\n");
+            let mut v = String::from("const srcIndex = new Map(JSON.parse('[\\\n");


Oh, crud.

The test cases pass, but this has a race condition. I'll open a PR to fix it in a few minutes.

…laumeGomez rustdoc-search: fix a race condition in search index loading `var` declare it in the global scope, and `const` does not. It needs to be declared in global scope. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const > const declarations do not create properties on [globalThis](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis) when declared at the top level of a script. Fixes a regression introduced by rust-lang#118910

rustdoc-search: fix a race condition in search index loading `var` declare it in the global scope, and `const` does not. It needs to be declared in global scope. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const > const declarations do not create properties on [globalThis](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis) when declared at the top level of a script. Fixes a regression introduced by rust-lang/rust#118910

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rustdoc] Use Map instead of Object for source files and search index #118910

[rustdoc] Use Map instead of Object for source files and search index #118910

GuillaumeGomez commented Dec 13, 2023

rustbot commented Dec 13, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

notriddle Dec 13, 2023

GuillaumeGomez Dec 13, 2023

notriddle Dec 13, 2023

GuillaumeGomez Dec 13, 2023

notriddle Dec 13, 2023

GuillaumeGomez Dec 13, 2023

notriddle Dec 14, 2023

notriddle commented Dec 13, 2023

This comment has been minimized.

bors commented Dec 13, 2023

bors commented Dec 13, 2023

This comment has been minimized.

rust-timer commented Dec 14, 2023

GuillaumeGomez commented Dec 14, 2023

notriddle commented Dec 14, 2023

bors commented Dec 14, 2023

GuillaumeGomez commented Dec 14, 2023

notriddle commented Dec 14, 2023 •

edited

Loading

GuillaumeGomez commented Dec 14, 2023

notriddle Dec 15, 2023

[rustdoc] Use Map instead of Object for source files and search index #118910

[rustdoc] Use Map instead of Object for source files and search index #118910

Conversation

GuillaumeGomez commented Dec 13, 2023

rustbot commented Dec 13, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notriddle commented Dec 13, 2023

This comment has been minimized.

bors commented Dec 13, 2023

bors commented Dec 13, 2023

This comment has been minimized.

rust-timer commented Dec 14, 2023

Overall result: no relevant changes - no action needed

GuillaumeGomez commented Dec 14, 2023

notriddle commented Dec 14, 2023

bors commented Dec 14, 2023

GuillaumeGomez commented Dec 14, 2023

notriddle commented Dec 14, 2023 • edited Loading

GuillaumeGomez commented Dec 14, 2023

Choose a reason for hiding this comment

notriddle commented Dec 14, 2023 •

edited

Loading