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

npm list checks added in #84586 regress wall time of running tidy #86147

Closed
the8472 opened this issue Jun 8, 2021 · 4 comments · Fixed by #86152
Closed

npm list checks added in #84586 regress wall time of running tidy #86147

the8472 opened this issue Jun 8, 2021 · 4 comments · Fixed by #86152
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@the8472
Copy link
Member

the8472 commented Jun 8, 2021

#84586 introduced some checks which execute npm list 4 times. They run unconditionally when executing ./x.py test ..., even if it is just to run tidy. On my system the added wall time exceeds that of running tidy itself.

I'll look into whether the checks themselves can be made be less wasteful. But if there are suggestions how to completely eliminate them or defer them while preserving their original purpose that would be great too.

npm list

CC @GuillaumeGomez can you explain the purpose of running this in should_run rather than erroring in run?

@jonas-schievink jonas-schievink added A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-rustdoc labels Jun 8, 2021
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jun 8, 2021

To know beforehand if the test should unconditionally be run or not. If the test is run and the npm package isn't installed, we panic.

To be noted that we could make it go down to 2 by switching the order of npm list: currently we check both "local" and "global". Switching the two of them will divide the number of calls by two.

cc @Mark-Simulacrum

@the8472
Copy link
Member Author

the8472 commented Jun 8, 2021

I don't have browser-ui-test installed, so it'll fail and no short-circuiting will happen regardless of order.

@GuillaumeGomez
Copy link
Member

In case you don't have it it'll check 4 times indeed.

@Mark-Simulacrum
Copy link
Member

Hm, this does seem annoying. It's possible we could add some lazily-evaluated default condition so that this never runs if we're not considering default-on steps (if defaults are on, then these npm lists are likely quite fast in comparison to the rest of the logic).

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jun 21, 2021
…Simulacrum

Lazify is_really_default condition in the RustdocGUI bootstrap step

The `RustdocGUI::should_run` condition spawns `npm list` several times which adds up to seconds of wall-time.
Evaluate the condition lazily to to keep `./x.py test tidy` and similar short-running tasks fast.

Fixes rust-lang#86147
@bors bors closed this as completed in e7a4f1e Jun 21, 2021
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants