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

Bustage due to recent self-profile changes #342

Closed
nnethercote opened this issue Feb 11, 2019 · 10 comments
Closed

Bustage due to recent self-profile changes #342

nnethercote opened this issue Feb 11, 2019 · 10 comments

Comments

@nnethercote
Copy link
Contributor

I get this when running the profile or bench commands:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error("invalid type: map, expected a sequence", line: 1, column: 19)'

The problem is this line: https://github.com/rust-lang-nursery/rustc-perf/blob/80826de37c486428f65a376d8aa0b5a8bb8dfcfe/collector/src/bin/rustc-perf-collector/execute.rs#L250

I think it assumes that the JSON is an array, but it's now an object (with keys/values).

I suspect rust-lang/rust#58085 is the cause.

@nnethercote
Copy link
Contributor Author

The performance of the new self-profile code is also unacceptable; it increases instruction counts by as much as 40%. See rust-lang/rust#58085 (comment)

@michaelwoerister
Copy link
Member

Huh, I don't know that rustc-perf actually did anything with the json output.

I think self-profiling shouldn't be done on perf.rlo yet until the data format has settled down.

@Mark-Simulacrum
Copy link
Member

We can definitely stop collecting the data; we don't actually do anything with it yet - just load it and re-serialize it.

@michaelwoerister
Copy link
Member

Yes, let's do this for now to unbreak things. The data will look differently in the future anyway.

@Mark-Simulacrum
Copy link
Member

Perf should now be fixed.

@michaelwoerister
Copy link
Member

Thanks, @Mark-Simulacrum!

@michaelwoerister
Copy link
Member

@Mark-Simulacrum: The compiler is still invoked with the -Z self-profile flag: https://github.com/rust-lang-nursery/rustc-perf/blob/56a77d04f6a73c7d8724ddde1d5628e28ab424e5/collector/src/bin/rustc-perf-collector/execute.rs#L233-L236

Given how much slower it makes the compiler in the current form, we should remove that too, I think.

@michaelwoerister
Copy link
Member

Also, I left some notes on how to make self-profiling more efficient here: rust-lang/rust#58372

@Mark-Simulacrum
Copy link
Member

Fixed the invocation with self-profile flag; I had missed that in my initial search... thanks!

@michaelwoerister
Copy link
Member

Thanks, @Mark-Simulacrum! Looks like perf.rlo is back to normal.

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

No branches or pull requests

3 participants