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

Investigate impact of removing shell32 dependency of libstd on Windows #56510

Closed
alexcrichton opened this issue Dec 4, 2018 · 9 comments
Closed
Labels
O-windows Operating system: Windows T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

An excellent blog post recently showed that removing the gdi32.dll dependency from LLVM speed up its test suite by a huge amount. For us libstd doesn't depend on gdi32.dll directly but it does depend on shell32.dll which depends on gdi32.dll (according to the blog post). Specificaly it looks like CommandLineToArgvW, the same function as LLVM, is used by us!

It'd be great to investigate removing the shell32 dependency from libstd (may be a few other functions here and there). We should run some tests on Windows to see if this improves rustc cycle times!

@alexcrichton alexcrichton added the O-windows Operating system: Windows label Dec 4, 2018
@Mark-Simulacrum Mark-Simulacrum added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Dec 4, 2018
@retep998
Copy link
Member

retep998 commented Dec 6, 2018

Based on my limited investigation, it's likely that replacing CommandLineToArgvW with a pure Rust equivalent should be good enough to get rid of the gdi32 dependency in things that only use libstd. So really someone just needs to write that pure Rust implementation which should be easy to do.

@Zoxc
Copy link
Contributor

Zoxc commented Dec 7, 2018

It would be nice to get rid of this in the compiler too. We spawn a lot of compiler instances during bootstrapping.

notriddle added a commit to notriddle/rust that referenced this issue Dec 10, 2018
bors added a commit that referenced this issue Dec 14, 2018
Remove dependency on shell32.dll

Closes #56510 if it works on MinGW (I've only tested it on MSVC).
@retep998
Copy link
Member

So now that the change has been implemented, have we actually investigated how much of an impact the change made?

@Eh2406
Copy link
Contributor

Eh2406 commented Dec 17, 2018

Perf, an lolbench are both linux only. So nether help with this question. So don't waste your time like I did.

@Zoxc
Copy link
Contributor

Zoxc commented Dec 20, 2018

Here are just number for run-pass on 8 cores / 16 threads on Windows 10 v1809:

Without the shell32 dependency:
333.012 325.587 320.650

With shell32:
310.852 319.165 323.645

@Mark-Simulacrum
Copy link
Member

For what it's worth, though there's no good way to differentiate PR-to-PR due to very high variance, it does look like there was a recent regression in PR build times:


image

@Zoxc
Copy link
Contributor

Zoxc commented Dec 20, 2018

I see that the CPU usage averages about 40% when running run-pass, so maybe we'd see some real improvements if we got rid of shell32.dll in the compiler too. Something is certainly holding things back.

@notriddle
Copy link
Contributor

@Zoxc

micha@DESKTOP-IIQA1VP MINGW64 ~/IdeaProjects/rust (master)
$ ldd build/x86_64-pc-windows-msvc/stage1/bin/rustc.exe | grep -i ntdll
        ntdll.dll => /c/WINDOWS/SYSTEM32/ntdll.dll (0x7ffbede90000)

micha@DESKTOP-IIQA1VP MINGW64 ~/IdeaProjects/rust (master)
$ ldd build/x86_64-pc-windows-msvc/stage1/bin/rustc.exe | grep -i gdi

micha@DESKTOP-IIQA1VP MINGW64 ~/IdeaProjects/rust (master)
$

@Zoxc
Copy link
Contributor

Zoxc commented Dec 25, 2018

Yeah, I saw that the compiler doesn't load gdi32.dll. So there's probably some other reason why compiletest doesn't load all the cores.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Operating system: Windows T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants