Skip to content

Remove JSON search file #2552

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

Merged
merged 3 commits into from
Mar 22, 2025

Conversation

GuillaumeGomez
Copy link
Member

Why bother having the search index in both a JSON and JS file when it always work with the JS file after all. :)

I took over #2425 to implement this.

cc @notriddle

@GuillaumeGomez
Copy link
Member Author

Rebased and fixed merge conflict.

@ehuss
Copy link
Contributor

ehuss commented Mar 21, 2025

I don't quite understand this PR. Doesn't this introduce the problem of the index loading not being async? Can you provide a little more commentary with the PRs to help explain what they are doing and why, along with some historical information to explain why it is the way it is originally?

@notriddle
Copy link
Contributor

notriddle commented Mar 21, 2025

When a script tag is added using the DOM API, it loads asynchronously. Parsing and running blocks the UI thread, but that's true with JSON.parse too.

@GuillaumeGomez
Copy link
Member Author

Exactly as @notriddle said. In short: it's doing exactly the same as the JSON loading was doing, except that we skip the JSON part and only keep the JS (because JSON loading doesn't work locally). It remains async though, hence why I needed to add onload callback.

@ehuss
Copy link
Contributor

ehuss commented Mar 22, 2025

I see, thanks!

Is it true that the fail-on-request-error lines in the goml files can be removed now? Can you do that here?

Also, can you also update this:

--- a/src/renderer/html_handlebars/search.rs
+++ b/src/renderer/html_handlebars/search.rs
@@ -60,7 +60,7 @@ pub fn create_files(
     let index = write_to_json(index, search_config, doc_urls)?;
     debug!("Writing search index ✓");
     if index.len() > 10_000_000 {
-        warn!("searchindex.json is very large ({} bytes)", index.len());
+        warn!("search index is very large ({} bytes)", index.len());
     }
 
     if search_config.copy_js {

@GuillaumeGomez
Copy link
Member Author

Is it true that the fail-on-request-error lines in the goml files can be removed now? Can you do that here?

I thought I did, gonna remove the remaining ones.

Also, can you also update this:

Sure!

@GuillaumeGomez GuillaumeGomez force-pushed the deduplicate-search-indexes branch from 88513d8 to 3cd717f Compare March 22, 2025 16:51
@GuillaumeGomez GuillaumeGomez force-pushed the deduplicate-search-indexes branch from 3cd717f to f54356d Compare March 22, 2025 16:56
@GuillaumeGomez
Copy link
Member Author

In the move-between test, the fail-on-request-error is still required, but not because of the search-index anymore. I updated the comment.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@ehuss ehuss added this pull request to the merge queue Mar 22, 2025
Merged via the queue into rust-lang:master with commit 3ed3024 Mar 22, 2025
12 checks passed
@GuillaumeGomez GuillaumeGomez deleted the deduplicate-search-indexes branch March 23, 2025 09:04
@sgued
Copy link

sgued commented Apr 1, 2025

Parsing and running blocks the UI thread, but that's true with JSON.parse too.

That's true, but you should also be aware that parsing JSON is much faster than parsing object literals. Given the size of the index, this is most likely measurable performance regression.

Note that this could also be improved by having the searchindex.js just use JSON.parse('{...}').

https://v8.dev/blog/cost-of-javascript-2019#json

@GuillaumeGomez
Copy link
Member Author

It's in my TODO list. We had this issue a long time ago in rustdoc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants