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

Add testing of collector profile_local. #702

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

nnethercote
Copy link
Contributor

Because testing is good.

@nnethercote
Copy link
Contributor Author

I've abused the && operator heavily in this PR. I'd be happy to hear suggestions about better ways to do things.

@Mark-Simulacrum
Copy link
Member

Hm, I think set -e should make us early exit if anything fails in the bash script -- would that work?

@nnethercote
Copy link
Contributor Author

set -e is a good idea. I also need to add kill $PING_LOOP_ID to the trap, to make sure it happens.

@nnethercote
Copy link
Contributor Author

The tests are failing due to an ICE when compiling collector:

error: internal compiler error: src/librustc_mir/monomorphize/collector.rs:629:54: collection encountered polymorphic constant
  --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.21/src/runtime/task/raw.rs:30:1

@nnethercote
Copy link
Contributor Author

The ICE is occurring also for the trivial change in #703. Might be a regression in the latest nightly rustc?

@Mark-Simulacrum
Copy link
Member

I think this is rust-lang/rust#74614

@Mark-Simulacrum
Copy link
Member

#704 should have fixed CI, I'm closing/reopening this to re-trigger it here.

@Mark-Simulacrum Mark-Simulacrum force-pushed the test-profile_local branch 2 times, most recently from 9cec8b3 to 13cef91 Compare July 22, 2020 14:25
@Mark-Simulacrum
Copy link
Member

Added some commits that are definitely necessary, but not sure what file it's not finding.

@nnethercote
Copy link
Contributor Author

I think I'll just remove the self-profiling testing, I forgot about needing all the measureme stuff. Just testing the eprintln profiling will still give us better coverage than we currently have, and catch a variety of potential problems.

@nnethercote nnethercote force-pushed the test-profile_local branch 2 times, most recently from b3dd51a to 6786461 Compare July 23, 2020 00:26
@nnethercote
Copy link
Contributor Author

I rebased to get #704 but somehow I'm still getting the ICE in the tests. /me scratches head

@Mark-Simulacrum
Copy link
Member

This happened to me multiple times somehow as well, generally I've felt like a number of times my pushes to this PR have just been entirely ignored (i.e., CI runs but on an old commit somehow). My only theory is that GitHub is having problems again..

Install latest nightly

It looks like the newly added CI job needs to swap out nightly for beta.

@nnethercote
Copy link
Contributor Author

It looks like the newly added CI job needs to swap out nightly for beta.

That would be it!

@nnethercote nnethercote reopened this Jul 23, 2020
@nnethercote
Copy link
Contributor Author

Progress: the eprintln profiling passed the test, but the perf-record (which I used to replace the self-profile) didn't, due to this:

collector error: Failed to profile 'futures' with PerfRecord, recorded: expected success, got exit code: 101

stderr=warning: package replacement is not used: https://github.com/rust-lang/crates.io-index#openssl:0.7.14
   Compiling futures v0.1.0 (/tmp/.tmpo4ALEK)
Error:
The instructions:u event is not supported.

The exact invocation is perf record --call-graph=dwarf --output=perf --freq=299 --event=cycles:u,instructions:u.

I might just drop the perf-record testing.

Also neaten up `ci/check-benchmarks.sh` by using `set -e`.
@Mark-Simulacrum
Copy link
Member

Presumably whatever VM GitHub Actions puts us in is insufficiently "native" for instruction counting, I guess? Odd that cycles (allegedly) works though.

@nnethercote nnethercote reopened this Jul 23, 2020
@nnethercote nnethercote merged commit dfe0669 into rust-lang:master Jul 23, 2020
@nnethercote nnethercote deleted the test-profile_local branch July 23, 2020 02:21
@nnethercote
Copy link
Contributor Author

This finishes up #681.

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.

2 participants