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

Crash with valgrind #188

Closed
KillTheMule opened this issue Aug 9, 2018 · 13 comments
Closed

Crash with valgrind #188

KillTheMule opened this issue Aug 9, 2018 · 13 comments
Labels
Bug Something isn't working right Investigate Requires further investigation Need Information Need further information from the submitter

Comments

@KillTheMule
Copy link
Contributor

I've tried analyzing my benchmark with valgrind, and I've experienced a crash. Seems like criterion was the culprit (to be precise, running the benchmark with the standard harness made the crash go away), so I wanted to bring this to your attention. Seems to me that analyzing a benchmark with profiling tools is something people generally would want to do.

Or maybe I've simply made a mistake? Anyways, details can be found here, if there's anything I should to to investigate or help fix it, please let me know :)

@bheisler
Copy link
Owner

bheisler commented Aug 9, 2018

Hey, thanks for the bug report!

Huh! Never seen this before, but I'm not too surprised. Criterion.rs does some odd stuff with unsafe memory access to speed up the benchmark analysis. I was never sure that code was correct, but I haven't been able to replace it with safe code yet. That would be my first guess, anyway.

@KillTheMule
Copy link
Contributor Author

Well, it doesn't need to be connected to unsafe, since it looks like a simple stray unwrap. I'll try to get the debug symbols, so the backtrace can actually help.

@KillTheMule
Copy link
Contributor Author

As a side note, I've also tried using perf, and that gave strange results with criterion. Using the standard harness made perf show usefull things. Right now, I can't pinpoint "strange results" any further, but if you want me to I'd investigate to give a better description.

@bheisler
Copy link
Owner

Any more information you can provide would be helpful, yeah.

You should be able to enable debugging information in the benchmarks by adding this to your Cargo.toml (I'm not sure if this only applies to your crate or to all dependencies, though):

[profile.bench]
debug = true

@Eh2406
Copy link
Contributor

Eh2406 commented Aug 25, 2018

Several of the unsafe are marked with fixme: when 22257 is fix and rust-lang/rust#22257 has been for a while, or move this impl into a private percentiles module when rust-lang/rfcs#735 is merged and it has been for a while. Meny other are to transmute a &[A] into a &Sample<A> witch can use Sample::new, witch at least centralizes the use of unsafe.

@nickbabcock
Copy link

Just wanted to chime in that I can reproduce OP's crash on all my rust programs (if a reproduction case is not desired, one can skip this)

#[macro_use]
extern crate criterion;

use criterion::Criterion;

fn criterion_benchmark(c: &mut Criterion) {
    c.bench_function("20", |b| b.iter(|| 20));
}

criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);

...

RUSTFLAGS="-g" cargo bench --no-run
RUST_BACKTRACE=1 valgrind --tool=callgrind ./target/release/bench-*

Results in the following execution

==82420== Callgrind, a call-graph generating cache profiler
==82420== Copyright (C) 2002-2017, and GNU GPL'd, by Josef Weidendorfer et al.
==82420== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==82420== Command: ./target/release/bench_hashes-8bae12da8c2d68de
==82420==
==82420== For interactive control, run 'callgrind_control -h'.
==82421==
==82421== Events    : Ir
==82421== Collected : 866743
==82421==
==82421== I   refs:      866,743
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', libcore/option.rs:345:21
stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>
  16: <unknown>
  17: __libc_start_main
  18: <unknown>
==82420==
==82420== Events    : Ir
==82420== Collected : 6550477
==82420==
==82420== I   refs:      6,550,477

@bheisler
Copy link
Owner

Thanks! That will help when I get around to looking into this.

@KillTheMule
Copy link
Contributor Author

I ran the example of @nickbabcock through per, using String::from("20") as the body to make it actually do some work. You can find both perf.data.* files at https://github.com/KillTheMule/tmp. I'm not sure it will help, though.

@bheisler bheisler added Bug Something isn't working right Investigate Requires further investigation labels Dec 5, 2018
@bheisler bheisler modified the milestone: Version 0.2.6 Dec 9, 2018
@bheisler
Copy link
Owner

bheisler commented Dec 9, 2018

I haven't been able to reproduce this issue. Could you confirm that it's still happening? I tried 0.2.4, 0.2.5 and the latest git revision without seeing this crash.

@bheisler bheisler added the Need Information Need further information from the submitter label Dec 9, 2018
@bheisler bheisler removed this from the Version 0.2.6 milestone Dec 9, 2018
@nickbabcock
Copy link

Sorry for my silence, I get the same error with @KillTheMule 's project.

git clone https://github.com/KillTheMule/tmp.git criterion.rs-188
cd criterion.rs-188
cargo bench --no-run
RUST_BACKTRACE=1 valgrind --tool=callgrind ./target/release/bench_tmp-*

I believe I have a pretty standard environment: Ubuntu 18.04, valgrind 3.13.0, rust 1.31.0

@bheisler
Copy link
Owner

I was finally able to reproduce this using an Ubuntu VM which did not have gnuplot installed. In my testing, I found that installing gnuplot fixed it, as did updating to Criterion.rs 0.2.6. Could you folks check if you're still having the problem with the latest version?

@nickbabcock
Copy link

Excellent, I can confirm that upgrading to criterion 0.2.6 fixes this issue. Instead of panicking, criterion will print out that gnuplot was not found. 👍

@KillTheMule
Copy link
Contributor Author

Took me some time, sorry for that, but updating to 0.2.7 fixed the issue indeed, thanks!

@bheisler bheisler closed this as completed Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working right Investigate Requires further investigation Need Information Need further information from the submitter
Projects
None yet
Development

No branches or pull requests

4 participants