Skip to content

Add support for building rustc-perf and running the collector on Windows #865

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 11 commits into from
May 27, 2021

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Apr 23, 2021

This PR implements the first two parts of #834: basic support for building the code on Windows and support for profiling using -Zself-profile.

Currently, running all tests works, other than a number of benchmarks that fail to build on my machine due to missing native dependencies (such as webrender).

The general strategy I took was to rewrite in Rust whenever convenient, otherwise falling back to a Windows equivalent of the unix-y tool.

Part of #834

@Mark-Simulacrum
Copy link
Member

It looks like CI is failing non-spuriously here - it'll currently fail even on master due to syn incremental being broken in rustc itself, though, so you won't get it fully green :)

Before we land this we should likely add a windows builder to CI to make sure we don't accidentally break support; at least just basic building (e.g. cargo check), but maybe something more involved would work too. I don't think we can use perf stat on Windows, so perhaps a self-profile collection on a few benchmarks would also help iron out any latent bugs.

Otherwise I think this approach looks really good to me - some duplication and expansion for sure, but nothing too surprising or unacceptable I think :) Thanks!

let target_dir = Component::Normal(OsStr::new("target"));

// Don't touch files in `target/`, since they're likely generated by build scripts and might be from a dependency.
if path.components().any(|component| component == target_dir) {
Copy link
Member

Choose a reason for hiding this comment

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

This would break if any subfolder is named target no?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's only true if the subfolder actually needs touching; I'm not sure that will happen in practice. Realistically we can likely get away with touching the crate "roots" only, but those are a bit annoying to identify.

@Mark-Simulacrum
Copy link
Member

Hey @wesleywiser -- I noticed this is still marked as [WIP] but wondering if that's no longer actually the case?

@wesleywiser wesleywiser mentioned this pull request May 18, 2021
6 tasks
@Mark-Simulacrum
Copy link
Member


2021-05-18T20:26:27.1254410Z [2021-05-18T20:26:27Z DEBUG collector::execute] applying patch add static arr item
2021-05-18T20:26:27.1255811Z [2021-05-18T20:26:27Z DEBUG collector::execute] applying add static arr item to "/tmp/.tmpV933YN"
2021-05-18T20:26:27.1330858Z error: patch failed: src/main.rs:65572
2021-05-18T20:26:27.1331599Z error: src/main.rs: patch does not apply
2021-05-18T20:26:27.1344711Z collector error: Failed to benchmark 'coercions', recorded: expected success, got exit status: 1

@wesleywiser wesleywiser changed the title [WIP] Add support for building rustc-perf and running the collector on Windows Add support for building rustc-perf and running the collector on Windows May 18, 2021
@wesleywiser
Copy link
Member Author

I believe this is ready for a final review. I've updated the title and description of the PR to match the current state and I've tested running a basic profile_local command on both Windows & Linux with success.

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

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

🎉

@wesleywiser wesleywiser merged commit d6d0634 into rust-lang:master May 27, 2021
@wesleywiser wesleywiser deleted the windows_support branch May 27, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants