diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 103f319c..944e1a78 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -19,7 +19,7 @@ jobs: uses: actions-rs/toolchain@v1.0.7 with: profile: minimal - toolchain: 1.64.0 + toolchain: 1.66.0 override: true components: rustfmt, clippy @@ -56,7 +56,7 @@ jobs: strategy: matrix: os: [ubuntu-latest, macos-latest] - toolchain: [stable, nightly, 1.64.0] + toolchain: [stable, nightly, 1.66.0] target: [ x86_64-unknown-linux-gnu, diff --git a/CHANGELOG.md b/CHANGELOG.md index 631a2c79..24a11de1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Fixed +- Fix the alignment of the collector and validate function (#255) + +### Changed +- Bump the MSRV to 1.66.0 (#255) + ## [0.13.0] - 2023-09-27 ### Changed diff --git a/Cargo.lock b/Cargo.lock index c9355b8d..d8e7c221 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19,14 +19,15 @@ checksum = "f26201604c87b1e01bd3d98f8d5d9a8fcbb815e8cedb41ffccbeb4bf593a35fe" [[package]] name = "ahash" -version = "0.8.3" +version = "0.8.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2c99f64d1e06488f620f932677e24bc6e2897582980441ae90a671415bd7ec2f" +checksum = "e89da841a80418a9b391ebaea17f5c112ffaaa96f621d2c285b5174da76b9011" dependencies = [ "cfg-if", "getrandom", "once_cell", "version_check", + "zerocopy", ] [[package]] @@ -38,6 +39,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "aligned-vec" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7e0966165eaf052580bd70eb1b32cb3d6245774c0104d1b2793e9650bf83b52a" +dependencies = [ + "equator", +] + [[package]] name = "anes" version = "0.1.6" @@ -317,6 +327,26 @@ version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a26ae43d7bcc3b814de94796a5e736d4029efb0ee900c12e2d54c993ad1a1e07" +[[package]] +name = "equator" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c35da53b5a021d2484a7cc49b2ac7f2d840f8236a286f84202369bd338d761ea" +dependencies = [ + "equator-macro", +] + +[[package]] +name = "equator-macro" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3bf679796c0322556351f287a51b49e48f7c4986e727b5dd78c972d30e2e16cc" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "equivalent" version = "1.0.1" @@ -680,6 +710,7 @@ dependencies = [ name = "pprof" version = "0.13.0" dependencies = [ + "aligned-vec", "backtrace", "cfg-if", "criterion", @@ -721,9 +752,9 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.67" +version = "1.0.89" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d433d9f1a3e8c1263d9456598b16fec66f4acc9a74dacffd35c7bb09b3a1328" +checksum = "f139b0662de085916d1fb67d2b4169d1addddda1919e696f3252b740b629986e" dependencies = [ "unicode-ident", ] @@ -1047,9 +1078,9 @@ dependencies = [ [[package]] name = "syn" -version = "2.0.37" +version = "2.0.43" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7303ef2c05cd654186cb250d29049a24840ca25d2747c25c0381c8d9e2f582e8" +checksum = "ee659fb5f3d355364e1f3e5bc10fb82068efbf824a1e9d1c9504244a6469ad53" dependencies = [ "proc-macro2", "quote", @@ -1311,3 +1342,23 @@ name = "windows_x86_64_msvc" version = "0.48.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed94fce61571a4006852b7389a063ab983c02eb1bb37b47f8272ce92d06d9538" + +[[package]] +name = "zerocopy" +version = "0.7.35" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1b9b4fd18abc82b8136838da5d50bae7bdea537c574d8dc1a34ed098d6c166f0" +dependencies = [ + "zerocopy-derive", +] + +[[package]] +name = "zerocopy-derive" +version = "0.7.35" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fa4f8080344d4671fb4e831a13ad1e68092748387dfc4f55e356242fae12ce3e" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] diff --git a/Cargo.toml b/Cargo.toml index 7d9f0378..f7b3bbd0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,7 +8,7 @@ description = "An internal perf tools for rust programs." repository = "https://github.com/tikv/pprof-rs" documentation = "https://docs.rs/pprof/" readme = "README.md" -rust-version = "1.64.0" # MSRV +rust-version = "1.66.0" # MSRV [features] default = ["cpp"] @@ -39,6 +39,7 @@ prost = { version = "0.12", optional = true } prost-derive = { version = "0.12", optional = true } protobuf = { version = "2.0", optional = true } criterion = {version = "0.5", optional = true} +aligned-vec = "0.6" [dependencies.symbolic-demangle] version = "12.1" diff --git a/src/addr_validate.rs b/src/addr_validate.rs index df3bf728..cd39bcac 100644 --- a/src/addr_validate.rs +++ b/src/addr_validate.rs @@ -69,6 +69,12 @@ fn open_pipe() -> nix::Result<()> { // `write()` will return an error the error number should be `EFAULT` in most // cases, but we regard all errors (except EINTR) as a failure of validation pub fn validate(addr: *const libc::c_void) -> bool { + // it's a short circuit for null pointer, as it'll give an error in + // `std::slice::from_raw_parts` if the pointer is null. + if addr.is_null() { + return false; + } + const CHECK_LENGTH: usize = 2 * size_of::<*const libc::c_void>() / size_of::(); // read data in the pipe diff --git a/src/collector.rs b/src/collector.rs index c05fb18b..6aa00425 100644 --- a/src/collector.rs +++ b/src/collector.rs @@ -8,6 +8,7 @@ use std::io::{Read, Seek, SeekFrom, Write}; use crate::frames::UnresolvedFrames; +use aligned_vec::AVec; use tempfile::NamedTempFile; pub const BUCKETS: usize = 1 << 12; @@ -148,6 +149,7 @@ pub struct TempFdArray { file: NamedTempFile, buffer: Box<[T; BUFFER_LENGTH]>, buffer_index: usize, + flush_n: usize, } impl TempFdArray { @@ -162,6 +164,7 @@ impl TempFdArray { file, buffer, buffer_index: 0, + flush_n: 0, }) } } @@ -175,6 +178,7 @@ impl TempFdArray { BUFFER_LENGTH * std::mem::size_of::(), ) }; + self.flush_n += 1; self.file.write_all(buf)?; Ok(()) @@ -192,10 +196,16 @@ impl TempFdArray { } fn try_iter(&self) -> std::io::Result> { - let mut file_vec = Vec::new(); + let size = BUFFER_LENGTH * self.flush_n * std::mem::size_of::(); + + let mut file_vec = AVec::with_capacity(std::mem::align_of::(), size); let mut file = self.file.reopen()?; - file.seek(SeekFrom::Start(0))?; - file.read_to_end(&mut file_vec)?; + + unsafe { + // it's safe as the capacity is initialized to `size`, and it'll be filled with `size` bytes + file_vec.set_len(size); + } + file.read_exact(&mut file_vec[0..size])?; file.seek(SeekFrom::End(0))?; Ok(TempFdArrayIterator { @@ -208,7 +218,7 @@ impl TempFdArray { pub struct TempFdArrayIterator<'a, T> { pub buffer: &'a [T], - pub file_vec: Vec, + pub file_vec: AVec, pub index: usize, } @@ -266,7 +276,7 @@ mod test_utils { use super::*; use std::collections::BTreeMap; - pub fn add_map(hashmap: &mut BTreeMap, entry: &Entry) { + pub fn add_map(hashmap: &mut BTreeMap, entry: &Entry) { match hashmap.get_mut(&entry.item) { None => { hashmap.insert(entry.item, entry.count); @@ -359,4 +369,57 @@ mod tests { } } } + + #[derive(Debug, Hash, Eq, PartialEq, PartialOrd, Ord, Default, Clone, Copy)] + struct AlignTest { + a: u16, + b: u32, + c: u64, + d: u64, + } + + // collector_align_test uses a bigger item to test the alignment of the collector + #[test] + fn collector_align_test() { + let mut collector = Collector::new().unwrap(); + let mut real_map = BTreeMap::new(); + + for item in 0..(1 << 12) * 4 { + for _ in 0..(item % 4) { + collector + .add( + AlignTest { + a: item as u16, + b: item as u32, + c: item as u64, + d: item as u64, + }, + 1, + ) + .unwrap(); + } + } + + collector.try_iter().unwrap().for_each(|entry| { + test_utils::add_map(&mut real_map, entry); + }); + + for item in 0..(1 << 12) * 4 { + let count = (item % 4) as isize; + let align_item = AlignTest { + a: item as u16, + b: item as u32, + c: item as u64, + d: item as u64, + }; + match real_map.get(&align_item) { + Some(value) => { + assert_eq!(count, *value); + } + None => { + assert_eq!(count, 0); + } + } + } + } } diff --git a/src/profiler.rs b/src/profiler.rs index d7314ea7..f14bda51 100644 --- a/src/profiler.rs +++ b/src/profiler.rs @@ -279,7 +279,8 @@ impl Drop for ErrnoProtector { target_arch = "riscv64", target_arch = "loongarch64" ))), - allow(unused_variables) + allow(unused_variables), + allow(clippy::unnecessary_cast) )] extern "C" fn perf_signal_handler( _signal: c_int,