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

Optimize libcxx-version for faster bootstrap startups #126467

Closed

Conversation

DaniPopes
Copy link
Contributor

Ref: #126423

time (c++ src/tools/libcxx-version/main.cpp && ./a.out && rm ./a.out) goes from ~180ms to ~50ms on my machine.

I tried doing this at compile time with #pragma message, however I couldn't find a way to reliably parse the output across GCC and Clang. Here's how that would've looked like:

// Include a small standard library header to define the version macros.
#include <cassert>

#define STR(x) XSTR(x)
#define XSTR(x) #x

#ifdef _GLIBCXX_RELEASE
    #pragma message "libstdc++ version: " STR(_GLIBCXX_RELEASE)
#elif defined(_LIBCPP_VERSION)
    #pragma message "libc++ version: " STR(_LIBCPP_VERSION)
#else
    #error "Couldn't recognize the standard library version."
#endif

cc @onur-ozkan as implementor in #125411

@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2024

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 14, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Jun 14, 2024

Good idea with the cstdio header. But we shouldn't even be running this everytime, it seems quite heavy as a sanity check. @onur-ozkan What about only running this check when you're actually about to download LLVM from CI? Therefore if you already have it downloaded, the check wouldn't be performed anymore.

@onur-ozkan
Copy link
Member

What about only running this check when you're actually about to download LLVM from CI? Therefore if you already have it downloaded, the check wouldn't be performed anymore.

That's already how it works

// Ensure that a compatible version of libstdc++ is available on the system when using `llvm.download-ci-llvm`.
#[cfg(not(feature = "bootstrap-self-test"))]
if !build.config.dry_run() && !build.build.is_msvc() && build.config.llvm_from_ci {

@Kobzol
Copy link
Contributor

Kobzol commented Jun 14, 2024

I meant that it would be executed literally just before trying to download LLVM, not just when llvm_from_ci is enabled. Now the check is executed everytime I run x, even though I have already downloaded LLVM from CI before, and my libstdc++ version is probably also not changing very often.

@onur-ozkan
Copy link
Member

time (c++ src/tools/libcxx-version/main.cpp && ./a.out && rm ./a.out) goes from ~180ms to ~50ms on my machine.

Are we sure these changes work without compatibility problems on old C++ compilers? The current code work on a fairly wide range of versions.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 14, 2024

Are we sure these changes work without compatibility problems on old C++ compilers? The current code work on a fairly wide range of versions.

The _GLIBCXX_RELEASE macro should be available anywhere in the compilation unit, even without any import, and cstdio should definitely work with any C++ compiler. In theory, it should be std::printf, but any C++ compiler that I know will also accept just printf here. So at least from my POV, it should be fine.

That being said, I don't think that we need to optimize this file at all, and instead we should just perform this check when it makes sense (i.e. when LLVM is being downloaded), and not on every run of bootstrap.

@DaniPopes
Copy link
Contributor Author

I realized that caching the output is probably the better solution, I opened this PR because it was an easier change (at least for me), feel free to close this if it's not wanted :)

@Kobzol Kobzol mentioned this pull request Jun 14, 2024
@onur-ozkan
Copy link
Member

We could refactor certain checks (not just the libcxx version) to be executed only once by using caching or stamp files, so they don't add overhead during active development.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 14, 2024

On my PC (64-bit Linux), the durations look like this:

libstdc++ check:   0.191737958
total sanity check: 0.203158588

So it seems like caching this specific check should be enough :)

@onur-ozkan
Copy link
Member

I just made some improvements on libcxx-version build in #126472 PR. I believe these changes alone should help without adding complexity to the sanity checks.

@onur-ozkan
Copy link
Member

I just made some improvements on libcxx-version build in #126472 PR. I believe these changes alone should help without adding complexity to the sanity checks.

Ofc, please correct me if I am wrong.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 14, 2024

I realized that caching the output is probably the better solution, I opened this PR because it was an easier change (at least for me), feel free to close this if it's not wanted :)

This is a good optimization, but ultimately we want to avoid running the C++ compiler in every bootstrap invocation, which #126472 does. So I would prefer that solution instead.

@DaniPopes DaniPopes closed this Jun 14, 2024
@DaniPopes DaniPopes deleted the optimize-libcxx-version branch June 14, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants