From fe47d5d7d032f324b71f03b64a292775159c2f3e Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 12 Jan 2023 13:22:17 -0500 Subject: [PATCH 01/21] Upgrade edition. --- memapi/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/memapi/Cargo.toml b/memapi/Cargo.toml index d3d49f8..6e1bf63 100644 --- a/memapi/Cargo.toml +++ b/memapi/Cargo.toml @@ -2,7 +2,7 @@ name = "pymemprofile_api" version = "0.1.0" authors = ["Itamar Turner-Trauring "] -edition = "2018" +edition = "2021" license = "Apache-2.0" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html From b12ff9563c5996870425d23e03c94404546419e2 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 13 Jan 2023 13:44:15 -0500 Subject: [PATCH 02/21] Start of linecaching interception strategy. --- Cargo.lock | 40 ++++++++++ memapi/Cargo.toml | 6 +- memapi/src/lib.rs | 2 +- memapi/src/linecache.rs | 159 +++++++++++++++++++++++++++++++++++----- memapi/src/python.rs | 6 +- 5 files changed, 187 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8c820e8..3b69e99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -407,6 +407,12 @@ dependencies = [ "hashbrown", ] +[[package]] +name = "indoc" +version = "1.0.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da2d6f23ffea9d7e76c53eee25dfb67bcd8fde7f1198b0855350698c9f07c780" + [[package]] name = "inferno" version = "0.11.13" @@ -735,10 +741,12 @@ dependencies = [ "libc", "libloading", "once_cell", + "parking_lot", "proc-maps", "proptest", "psutil", "pyo3", + "rusty-fork", "serde", "tempfile", ] @@ -750,11 +758,14 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "268be0c73583c183f2b14052337465768c07726936a260f480f0857cb95ba543" dependencies = [ "cfg-if", + "indoc", "libc", "memoffset", "parking_lot", "pyo3-build-config", "pyo3-ffi", + "pyo3-macros", + "unindent", ] [[package]] @@ -777,6 +788,29 @@ dependencies = [ "pyo3-build-config", ] +[[package]] +name = "pyo3-macros" +version = "0.17.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "94144a1266e236b1c932682136dc35a9dee8d3589728f68130c7c3861ef96b28" +dependencies = [ + "proc-macro2", + "pyo3-macros-backend", + "quote", + "syn", +] + +[[package]] +name = "pyo3-macros-backend" +version = "0.17.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c8df9be978a2d2f0cdebabb03206ed73b11314701a5bfe71b0d753b81997777f" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "quick-error" version = "1.2.3" @@ -1082,6 +1116,12 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dcc811dc4066ac62f84f11307873c4850cb653bfa9b1719cee2bd2204a4bc5dd" +[[package]] +name = "unindent" +version = "0.1.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1766d682d402817b5ac4490b3c3002d91dfa0d22812f341609f97b08757359c" + [[package]] name = "version_check" version = "0.9.4" diff --git a/memapi/Cargo.toml b/memapi/Cargo.toml index 6e1bf63..e19a0d7 100644 --- a/memapi/Cargo.toml +++ b/memapi/Cargo.toml @@ -18,6 +18,7 @@ once_cell = "1.17" libloading = "0.7" libc = "0.2" serde = {version = "1", features = ["derive"] } +parking_lot = "0.12.1" [dependencies.inferno] version = "0.11" @@ -30,8 +31,6 @@ features = ["memory", "process"] [dependencies.pyo3] version = "0.17" -default-features = false -features = [] [target.'cfg(target_os = "linux")'.dependencies] cgroups-rs = "0.2.11" @@ -40,8 +39,9 @@ cgroups-rs = "0.2.11" proptest = "1.0" proc-maps = "0.3.0" tempfile = "3.3.0" +rusty-fork = "0.3.0" [features] default = [] # Optimize for the production version of Fil. -fil4prod = [] \ No newline at end of file +fil4prod = [] diff --git a/memapi/src/lib.rs b/memapi/src/lib.rs index 22cbf9a..eddf7c7 100644 --- a/memapi/src/lib.rs +++ b/memapi/src/lib.rs @@ -5,7 +5,7 @@ pub mod linecache; pub mod memorytracking; pub mod mmap; pub mod oom; -mod python; +pub mod python; mod rangemap; pub mod util; diff --git a/memapi/src/linecache.rs b/memapi/src/linecache.rs index 673a1ba..4ffe9f2 100644 --- a/memapi/src/linecache.rs +++ b/memapi/src/linecache.rs @@ -1,13 +1,15 @@ //! A line caching library, a bit like Python's linecache. +use ahash::RandomState; +use parking_lot::Mutex; +use pyo3::{prelude::*, types::PyString}; use std::{ collections::HashMap, fs::File, - io::{BufRead, BufReader}, + io::{BufRead, BufReader, Cursor, Read}, + sync::Arc, }; -use ahash::RandomState; - /// Store a cache of files in memory, allow easy reading of lines. #[derive(Default)] pub struct LineCacher { @@ -17,6 +19,32 @@ pub struct LineCacher { static EMPTY_STRING: String = String::new(); impl LineCacher { + /// Add lines for a particular filename, if it's not already added, returning an Entry. + fn maybe_add_file_data<'a, GR: FnOnce() -> Option>>( + &'a mut self, + filename: &str, + get_data: GR, + ) -> &'a Vec { + self.file_lines + .entry(filename.to_string()) + .or_insert_with(|| { + get_data() + .map(|r| { + BufReader::new(r) + .lines() + .map(|l| l.unwrap_or_else(|_| EMPTY_STRING.clone())) + .collect() + }) + .unwrap_or_else(Vec::new) + }) + } + + /// Add a string-of-lines for a particular filename. + fn add_string(&mut self, filename: &str, lines: &str) { + let lines = Cursor::new(lines.to_string().into_bytes()); + self.maybe_add_file_data(filename, || Some(Box::new(lines))); + } + /// Get the source code line for the given file. If the file doesn't exist /// or line number is too big, an empty string is returned. Line endings are /// stripped. @@ -24,31 +52,108 @@ impl LineCacher { if line_number == 0 { return &EMPTY_STRING; } - let entry = - self.file_lines - .entry(filename.to_string()) - .or_insert_with(|| -> Vec { - File::open(filename) - .map(|f| { - BufReader::new(f) - .lines() - .map(|l| l.unwrap_or_else(|_| EMPTY_STRING.clone())) - .collect() - }) - .unwrap_or_else(|_| vec![]) - }); + let entry = self.maybe_add_file_data(filename, || { + File::open(filename).ok().map(|f| { + let b: Box = Box::new(f); + b + }) + }); entry.get(line_number - 1).unwrap_or(&EMPTY_STRING) } } +/// Wrapper around Python's linecache.cache dictionary that captures manual +/// writes. +/// +/// In particular, IPython/Jupyter cells will get added there, and so we want to +/// make sure we have access to them in our linecache so we can create correct +/// tracebacks. +#[pyclass] +struct PyLineCacheWrapper { + py_linecache: PyObject, + rust_linecache: Arc>, +} + +impl PyLineCacheWrapper { + fn new(py_linecache: &PyAny) -> Self { + // TODO populate Rust linecache with non-filesystem cached data. + // TODO and write test. + Self { + py_linecache: py_linecache.into(), + rust_linecache: Arc::new(Mutex::new(LineCacher::default())), + } + } +} + +#[pymethods] +impl PyLineCacheWrapper { + /// Pass through pretty much everything to the underyling dict: + fn __getattribute__(slf: PyRef<'_, Self>, attr: &PyString) -> PyResult> { + slf.py_linecache.getattr(slf.py(), attr) + } + + /// __setitem__ is only code we handle specially: + fn __setitem__(slf: PyRef<'_, Self>, attr: &PyAny, value: &PyAny) -> PyResult<()> { + // TODO if entry is not a file, added it to rust cache. + // TODO and test. + println!("ADDING {} to linecache", attr); + let py = slf.py(); + slf.py_linecache + .getattr(py, "__setitem__")? + .call(py, (attr, value), None)?; + Ok(()) + } +} + +/// Make sure changes to the Python linecache end up in our cache, in particular +/// for Jupyter which manually adds item to Python's cache. +fn monkeypatch_python_linecache() -> PyResult<()> { + // TODO write test showing this changes the global linecache. + Python::with_gil(|py| { + let linecache = py.import("linecache")?; + let py_cache = linecache.getattr("cache")?; + let wrapper = PyLineCacheWrapper::new(py_cache); + linecache.setattr("cache", Py::new(py, wrapper)?)?; + Ok(()) + }) +} + +/// Get access to the LineCacher that is attached to `linecache` Python module +/// after calling `monkeypatch_python_linecache()`. +/// +/// If none is installed, a new one will be installed. +/// +/// If it's impossible to install for some reason, a temporary one-off will be +/// created, since at least filesystem tracebacks can still be used. +pub fn run_with_linecache(closure: F) { + // TODO write test showing this gets existing object attached to global linecache. + // TODO write test showing this attaches new one if global linecache is missing this object. + fn get_wrapper(py: Python<'_>) -> PyResult>> { + let linecache = py.import("linecache")?; + let wrapper = linecache.getattr("cache")?; + let wrapper: &PyCell = wrapper.downcast()?; + Ok(wrapper.try_borrow()?.rust_linecache.clone()) + } + + let linecacher = Python::with_gil(|py| { + get_wrapper(py) + .or_else(|_| { + monkeypatch_python_linecache(); + get_wrapper(py) + }) + .unwrap_or_else(|_| Arc::new(Mutex::new(LineCacher::default()))) + }); + closure(&linecacher.lock()); +} + #[cfg(test)] mod tests { + use super::*; + use rusty_fork::rusty_fork_test; use std::io::Write; - use super::LineCacher; - #[test] - fn linecache() { + fn linecacher() { let mut cache = LineCacher::default(); // Non-existent file @@ -69,4 +174,20 @@ mod tests { assert_eq!(cache.get_source_line(path, 2), "def"); assert_eq!(cache.get_source_line(path, 3), "ghijk"); } + + #[test] + fn linecacher_add_string() { + let mut cache = LineCacher::default(); + cache.add_string("file1", "a\nb"); + cache.add_string("file2", "c\nd"); + assert_eq!(cache.get_source_line("file1", 1), "a"); + assert_eq!(cache.get_source_line("file1", 2), "b"); + assert_eq!(cache.get_source_line("file2", 1), "c"); + assert_eq!(cache.get_source_line("file2", 2), "d"); + } + + rusty_fork_test! { + //#[test] + //fn + } } diff --git a/memapi/src/python.rs b/memapi/src/python.rs index 448e3a4..610e53a 100644 --- a/memapi/src/python.rs +++ b/memapi/src/python.rs @@ -1,7 +1,7 @@ -// Interactions with Python APIs. +//! Interactions with Python APIs. + use once_cell::sync::Lazy; -use pyo3::prelude::*; -use pyo3::types::PyModule; +use pyo3::{types::PyModule, Python}; // Return the filesystem path of the stdlib's runpy module. pub fn get_runpy_path() -> &'static str { From 04ea3b3dcefc4e1854f12e6960ef369bf48e2b6e Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 16 Jan 2023 16:39:07 -0500 Subject: [PATCH 03/21] Start refactoring how flamegraphs are generated so that we can use Python linecache in the rendering. --- memapi/src/flamegraph.rs | 331 ++++++++++++++++++++--------------- memapi/src/memorytracking.rs | 127 ++++---------- 2 files changed, 224 insertions(+), 234 deletions(-) diff --git a/memapi/src/flamegraph.rs b/memapi/src/flamegraph.rs index 334a9e1..10e775f 100644 --- a/memapi/src/flamegraph.rs +++ b/memapi/src/flamegraph.rs @@ -1,8 +1,13 @@ -use std::{fs, io::Write, path::Path}; +use std::{borrow::Cow, fs, io::Write, path::Path}; use inferno::flamegraph; use itertools::Itertools; +use crate::{ + linecache::LineCacher, + memorytracking::{Callstack, FunctionLocations}, +}; + /// Filter down to top 99% of samples. /// /// 1. Empty samples are dropped. @@ -59,171 +64,209 @@ pub fn write_lines>(lines: I, path: &Path) -> std Ok(()) } -/// Write a flamegraph SVG to disk, given lines in summarized format. -pub fn write_flamegraph>( - lines: I, - path: &Path, - reversed: bool, - title: &str, - subtitle: &str, - count_name: &str, - to_be_post_processed: bool, -) -> Result<(), Box> { - let flamegraph = get_flamegraph( - lines, - reversed, - title, - subtitle, - count_name, - to_be_post_processed, - )?; - let mut file = std::fs::File::create(path)?; - file.write_all(&flamegraph)?; - Ok(()) +/// A strategy for cleaning up callstacks before rendering them to text. +pub trait CallstackCleaner { + fn cleanup(&self, callstack: &Callstack) -> Cow; +} + +/// The data needed to create a flamegraph. +pub struct FlamegraphCallstacks { + data: D, + functions: FL, + callstack_cleaner: UC, } -/// Low-level interface for writing flamegraphs with post-processing: -pub fn get_flamegraph_with_options>( - lines: I, - to_be_post_processed: bool, - mut options: flamegraph::Options, - // special cased because it needs to be psot-processed: - subtitle: Option<&str>, -) -> Result, Box> { - let mut output = vec![]; - let lines: Vec = lines.into_iter().collect(); - match flamegraph::from_lines(&mut options, lines.iter().map(|s| s.as_ref()), &mut output) { - Err(e) => Err(format!("{}", e).into()), - Ok(_) => { - if to_be_post_processed { - // Replace with real subtitle. - let data = String::from_utf8(output)?; - let data = if let Some(subtitle) = subtitle { - data.replace("__FIL-SUBTITLE-HERE__", subtitle) - } else { - data - }; - // Restore normal semi-colons. - let data = data.replace('\u{ff1b}', ";"); - // Restore (non-breaking) spaces. - let data = data.replace('\u{12e4}', "\u{00a0}"); - // Get rid of empty-line markers: - let data = data.replace('\u{2800}', ""); - output = data.as_bytes().to_vec(); +impl<'a, D, FL, UC> FlamegraphCallstacks +where + &'a D: IntoIterator, + D: 'a, + FL: FunctionLocations, + UC: CallstackCleaner, +{ + pub fn new(data: D, functions: FL, callstack_cleaner: UC) -> Self { + Self { + data, + functions, + callstack_cleaner, + } + } + + /// Create iterator over the line-based string format parsed by the inferno + /// crate. + pub fn to_lines(&'a self, to_be_post_processed: bool) -> impl Iterator + 'a { + let by_call = (&self.data).into_iter(); + let mut linecache = LineCacher::default(); + by_call.map(move |(callstack, size)| { + format!( + "{} {}", + (self.update_callstack)(callstack).as_string( + to_be_post_processed, + &self.functions, + ";", + &mut linecache, + ), + size, + ) + }) + } + + /// Low-level interface for writing flamegraphs with post-processing: + pub fn get_flamegraph_with_options( + &self, + to_be_post_processed: bool, + mut options: flamegraph::Options, + // special cased because it needs to be psot-processed: + subtitle: Option<&str>, + ) -> Result, Box> { + let lines = self.to_lines(to_be_post_processed); + let mut output = vec![]; + let lines: Vec = lines.into_iter().collect(); + match flamegraph::from_lines(&mut options, lines.iter().map(|s| s.as_ref()), &mut output) { + Err(e) => Err(format!("{}", e).into()), + Ok(_) => { + if to_be_post_processed { + // Replace with real subtitle. + let data = String::from_utf8(output)?; + let data = if let Some(subtitle) = subtitle { + data.replace("__FIL-SUBTITLE-HERE__", subtitle) + } else { + data + }; + // Restore normal semi-colons. + let data = data.replace('\u{ff1b}', ";"); + // Restore (non-breaking) spaces. + let data = data.replace('\u{12e4}', "\u{00a0}"); + // Get rid of empty-line markers: + let data = data.replace('\u{2800}', ""); + output = data.as_bytes().to_vec(); + } + Ok(output) } - Ok(output) } } -} -/// Write a flamegraph SVG to disk, given lines in summarized format. -pub fn get_flamegraph>( - lines: I, - reversed: bool, - title: &str, - subtitle: &str, - count_name: &str, - to_be_post_processed: bool, -) -> Result, Box> { - let title = format!("{}{}", title, if reversed { ", Reversed" } else { "" },); - let mut options = flamegraph::Options::default(); - options.title = title; - options.count_name = count_name.to_string(); - options.font_size = 16; - options.font_type = "monospace".to_string(); - options.frame_height = 22; - options.reverse_stack_order = reversed; - options.color_diffusion = true; - options.direction = flamegraph::Direction::Inverted; - options.min_width = 0.2; - // Maybe disable this some day; but for now it makes debugging much - // easier: - options.pretty_xml = true; - if to_be_post_processed { - // Can't put structured text into subtitle, so have to do a hack. - options.subtitle = Some("__FIL-SUBTITLE-HERE__".to_string()); + /// Write a flamegraph SVG to disk, given lines in summarized format. + pub fn get_flamegraph( + &self, + reversed: bool, + title: &str, + subtitle: &str, + count_name: &str, + to_be_post_processed: bool, + ) -> Result, Box> { + let title = format!("{}{}", title, if reversed { ", Reversed" } else { "" },); + let mut options = flamegraph::Options::default(); + options.title = title; + options.count_name = count_name.to_string(); + options.font_size = 16; + options.font_type = "monospace".to_string(); + options.frame_height = 22; + options.reverse_stack_order = reversed; + options.color_diffusion = true; + options.direction = flamegraph::Direction::Inverted; + options.min_width = 0.2; + // Maybe disable this some day; but for now it makes debugging much + // easier: + options.pretty_xml = true; + if to_be_post_processed { + // Can't put structured text into subtitle, so have to do a hack. + options.subtitle = Some("__FIL-SUBTITLE-HERE__".to_string()); + } + self.get_flamegraph_with_options(to_be_post_processed, options, Some(subtitle)) } - get_flamegraph_with_options(lines, to_be_post_processed, options, Some(subtitle)) -} -/// Write .prof, -source.prof, .svg and -reversed.svg files for given lines. -pub fn write_flamegraphs( - directory_path: &Path, - base_filename: &str, - title: &str, - subtitle: &str, - count_name: &str, - to_be_post_processed: bool, - get_lines: F, -) where - I: IntoIterator, - F: Fn(bool) -> I, // (to_be_post_processed) -> lines -{ - if !directory_path.exists() { - fs::create_dir_all(directory_path) - .expect("=fil-profile= Couldn't create the output directory."); - } else if !directory_path.is_dir() { - panic!("=fil-profile= Output path must be a directory."); + /// Write a flamegraph SVG to disk. + pub fn write_flamegraph( + &self, + path: &Path, + reversed: bool, + title: &str, + subtitle: &str, + count_name: &str, + to_be_post_processed: bool, + ) -> Result<(), Box> { + let flamegraph = + self.get_flamegraph(reversed, title, subtitle, count_name, to_be_post_processed)?; + let mut file = std::fs::File::create(path)?; + file.write_all(&flamegraph)?; + Ok(()) } - let raw_path_without_source_code = directory_path.join(format!("{}.prof", base_filename)); + /// Write .prof, -source.prof, .svg and -reversed.svg files for given lines. + pub fn write_flamegraphs( + &self, + directory_path: &Path, + base_filename: &str, + title: &str, + subtitle: &str, + count_name: &str, + to_be_post_processed: bool, + ) { + if !directory_path.exists() { + fs::create_dir_all(directory_path) + .expect("=fil-profile= Couldn't create the output directory."); + } else if !directory_path.is_dir() { + panic!("=fil-profile= Output path must be a directory."); + } - let raw_path_with_source_code = directory_path.join(format!("{}-source.prof", base_filename)); + let raw_path_without_source_code = directory_path.join(format!("{}.prof", base_filename)); - // Always write .prof file without source code, for use by tests and - // other automated post-processing. - if let Err(e) = write_lines(get_lines(false), &raw_path_without_source_code) { - eprintln!("=fil-profile= Error writing raw profiling data: {}", e); - return; - } + let raw_path_with_source_code = + directory_path.join(format!("{}-source.prof", base_filename)); - // Optionally write version with source code for SVGs, if we're using - // source code. - if to_be_post_processed { - if let Err(e) = write_lines(get_lines(true), &raw_path_with_source_code) { + // Always write .prof file without source code, for use by tests and + // other automated post-processing. + if let Err(e) = write_lines(self.to_lines(false), &raw_path_without_source_code) { eprintln!("=fil-profile= Error writing raw profiling data: {}", e); return; } - } - let svg_path = directory_path.join(format!("{}.svg", base_filename)); - match write_flamegraph( - get_lines(to_be_post_processed), - &svg_path, - false, - title, - subtitle, - count_name, - to_be_post_processed, - ) { - Ok(_) => { - eprintln!("=fil-profile= Wrote flamegraph to {:?}", svg_path); + // Optionally write version with source code for SVGs, if we're using + // source code. + if to_be_post_processed { + if let Err(e) = write_lines(self.to_lines(true), &raw_path_with_source_code) { + eprintln!("=fil-profile= Error writing raw profiling data: {}", e); + return; + } } - Err(e) => { - eprintln!("=fil-profile= Error writing SVG: {}", e); + + let svg_path = directory_path.join(format!("{}.svg", base_filename)); + match self.write_flamegraph( + &svg_path, + false, + title, + subtitle, + count_name, + to_be_post_processed, + ) { + Ok(_) => { + eprintln!("=fil-profile= Wrote flamegraph to {:?}", svg_path); + } + Err(e) => { + eprintln!("=fil-profile= Error writing SVG: {}", e); + } } - } - let svg_path = directory_path.join(format!("{}-reversed.svg", base_filename)); - match write_flamegraph( - get_lines(to_be_post_processed), - &svg_path, - true, - title, - subtitle, - count_name, - to_be_post_processed, - ) { - Ok(_) => { - eprintln!("=fil-profile= Wrote flamegraph to {:?}", svg_path); + let svg_path = directory_path.join(format!("{}-reversed.svg", base_filename)); + match self.write_flamegraph( + &svg_path, + true, + title, + subtitle, + count_name, + to_be_post_processed, + ) { + Ok(_) => { + eprintln!("=fil-profile= Wrote flamegraph to {:?}", svg_path); + } + Err(e) => { + eprintln!("=fil-profile= Error writing SVG: {}", e); + } } - Err(e) => { - eprintln!("=fil-profile= Error writing SVG: {}", e); + if to_be_post_processed { + // Don't need this file, and it'll be quite big, so delete it. + let _ = std::fs::remove_file(raw_path_with_source_code); } } - if to_be_post_processed { - // Don't need this file, and it'll be quite big, so delete it. - let _ = std::fs::remove_file(raw_path_with_source_code); - } } #[cfg(test)] diff --git a/memapi/src/memorytracking.rs b/memapi/src/memorytracking.rs index 998a94f..a80dea8 100644 --- a/memapi/src/memorytracking.rs +++ b/memapi/src/memorytracking.rs @@ -1,5 +1,7 @@ use crate::flamegraph::filter_to_useful_callstacks; use crate::flamegraph::write_flamegraphs; +use crate::flamegraph::CallstackCleaner; +use crate::flamegraph::FlamegraphCallstacks; use crate::linecache::LineCacher; use crate::python::get_runpy_path; @@ -41,13 +43,19 @@ struct FunctionLocation { function_name: String, } +/// The clone should be cheap, ideally, so probably an immutable data structures +/// would be good. pub trait FunctionLocations { fn get_function_and_filename(&self, id: FunctionId) -> (&str, &str); + + // Like Clone.clone(), but should be cheap. + fn cheap_clone(&self) -> Self; } /// Stores FunctionLocations, returns a FunctionId #[derive(Clone)] pub struct VecFunctionLocations { + // TODO switch to immutable vec functions: Vec, } @@ -80,6 +88,12 @@ impl FunctionLocations for VecFunctionLocations { let location = &self.functions[id.0 as usize]; (&location.function_name, &location.filename) } + + fn cheap_clone(&self) -> Self { + Self { + functions: self.functions.clone(), + } + } } /// Either the line number, or the bytecode index needed to get it. @@ -186,10 +200,10 @@ impl Callstack { callstack_id } - pub fn as_string( + pub fn as_string( &self, to_be_post_processed: bool, - functions: &dyn FunctionLocations, + functions: &FL, separator: &'static str, linecache: &mut LineCacher, ) -> String { @@ -356,8 +370,13 @@ impl Allocation { } } -fn same_callstack(callstack: &Callstack) -> Cow { - Cow::Borrowed(callstack) +/// A CallstackCleaner that leaves the callstack unchanged. +struct IdentityCleaner; + +impl CallstackCleaner for IdentityCleaner { + fn cleanup(&self, callstack: &Callstack) -> Cow { + Cow::Borrowed(callstack) + } } /// The main data structure tracking everything. @@ -598,10 +617,10 @@ impl AllocationTracker { /// Combine Callstacks and make them human-readable. Duplicate callstacks /// have their allocated memory summed. fn combine_callstacks( - &self, + &mut self, // If false, will do the current allocations: peak: bool, - ) -> HashMap { + ) -> FlamegraphCallstacks, FL, IdentityCleaner> { // Would be nice to validate if data is consistent. However, there are // edge cases that make it slightly inconsistent (e.g. see the // unexpected code path in add_allocation() above), and blowing up @@ -615,96 +634,24 @@ impl AllocationTracker { // flamegraph (which currently loads EVERYTHING into memory), just do // the top 99% of allocations. let callstacks = if peak { + self.check_if_new_peak(); &self.peak_memory_usage } else { &self.current_memory_usage }; let sum = callstacks.iter().sum(); - filter_to_useful_callstacks(callstacks.iter().enumerate(), sum) - .into_iter() - .map(|(k, v)| (k as CallstackId, v)) - .collect() - } - - /// Dump all callstacks in peak memory usage to various files describing the - /// memory usage. - pub fn dump_peak_to_flamegraph(&mut self, path: &str) { - self.dump_to_flamegraph(path, true, "peak-memory", "Peak Tracked Memory Usage", true); - } - - pub fn to_lines<'a, F>( - &'a self, - peak: bool, - to_be_post_processed: bool, - update_callstack: F, - ) -> impl ExactSizeIterator + '_ - where - F: Fn(&Callstack) -> Cow + 'a, - { - let by_call = self.combine_callstacks(peak).into_iter(); let id_to_callstack = self.interner.get_reverse_map(); - let mut linecache = LineCacher::default(); - by_call.map(move |(callstack_id, size)| { - format!( - "{} {}", - update_callstack(id_to_callstack.get(&callstack_id).unwrap()).as_string( - to_be_post_processed, - &self.functions, - ";", - &mut linecache, - ), - size, - ) - }) - } - - fn dump_to_flamegraph( - &mut self, - path: &str, - peak: bool, - base_filename: &str, - title: &str, - to_be_post_processed: bool, - ) { - // First, make sure peaks are correct: - self.check_if_new_peak(); - - // Print warning if we're missing allocations. - #[cfg(not(feature = "fil4prod"))] - { - let allocated_bytes = if peak { - self.peak_allocated_bytes - } else { - self.current_allocated_bytes - }; - if self.missing_allocated_bytes > 0 { - eprintln!("=fil-profile= WARNING: {:.2}% ({} bytes) of tracked memory somehow disappeared. If this is a small percentage you can just ignore this warning, since the missing allocations won't impact the profiling results. If the % is high, please run `export FIL_DEBUG=1` to get more output', re-run Fil on your script, and then file a bug report at https://github.com/pythonspeed/filprofiler/issues/new", self.missing_allocated_bytes as f64 * 100.0 / allocated_bytes as f64, self.missing_allocated_bytes); - } - if self.failed_deallocations > 0 { - eprintln!("=fil-profile= WARNING: Encountered {} deallocations of untracked allocations. A certain number are expected in normal operation, of allocations created before Fil started tracking, and even more if you're using the Fil API to turn tracking on and off.", self.failed_deallocations); - } - } - - eprintln!("=fil-profile= Preparing to write to {}", path); - let directory_path = Path::new(path); - - let title = format!( - "{} ({:.1} MiB)", - title, - self.peak_allocated_bytes as f64 / (1024.0 * 1024.0) - ); - #[cfg(not(feature = "fil4prod"))] - let subtitle = r#"Made with the Fil profiler. Try it on your code!"#; - #[cfg(feature = "fil4prod")] - let subtitle = r#"Made with the Fil4prod profiler. Try it on your code!"#; - write_flamegraphs( - directory_path, - base_filename, - &title, - subtitle, - "bytes", - to_be_post_processed, - |tbpp| self.to_lines(peak, tbpp, same_callstack), + FlamegraphCallstacks::new( + filter_to_useful_callstacks(callstacks.iter().enumerate(), sum) + .into_iter() + .filter_map(|(k, v)| { + id_to_callstack + .get(&(k as CallstackId)) + .map(|cs| ((**cs).clone(), v)) + }) + .collect(), + self.functions.cheap_clone(), + IdentityCleaner {}, ) } From d1eac27688753d5f40342bb10bf9521472994cfc Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 17 Jan 2023 12:45:23 -0500 Subject: [PATCH 04/21] Finish up refactor that will allow using Python linecache. --- filpreload/src/lib.rs | 69 +++++++++++++++++++++++++++++++++--- memapi/src/flamegraph.rs | 12 +++---- memapi/src/memorytracking.rs | 44 +++++++++++------------ 3 files changed, 91 insertions(+), 34 deletions(-) diff --git a/filpreload/src/lib.rs b/filpreload/src/lib.rs index 05ca54c..d42fcbf 100644 --- a/filpreload/src/lib.rs +++ b/filpreload/src/lib.rs @@ -8,6 +8,7 @@ use pymemprofile_api::oom::{InfiniteMemory, OutOfMemoryEstimator, RealMemoryInfo use std::cell::RefCell; use std::ffi::CStr; use std::os::raw::{c_char, c_int, c_void}; +use std::path::Path; #[macro_use] extern crate lazy_static; @@ -152,7 +153,23 @@ fn add_allocation( if oom { // Uh-oh, we're out of memory. - allocations.oom_dump(); + eprintln!( + "=fil-profile= We'll try to dump out SVGs. Note that no HTML file will be written." + ); + let default_path = allocations.default_path.clone(); + // Release the lock, since dumping the flamegraph will reacquire it: + drop(tracker_state); + + dump_to_flamegraph( + &default_path, + false, + "out-of-memory", + "Current allocations at out-of-memory time", + false, + ); + unsafe { + libc::_exit(53); + } }; Ok(()) } @@ -180,11 +197,55 @@ fn reset(default_path: String) { tracker_state.allocations.reset(default_path); } +fn dump_to_flamegraph( + path: &str, + peak: bool, + base_filename: &str, + title: &str, + to_be_post_processed: bool, +) { + // In order to render the flamegraph, we want to load source code using + // Python's linecache. That means calling into Python, which might release + // the GIL, allowing another thread to run, and it will try to allocation + // and hit the TRACKER_STATE mutex. And now we're deadlocked. So we make + // sure flamegraph rendering does not require TRACKER_STATE to be locked. + let (allocated_bytes, flamegraph_callstacks) = { + let mut tracker_state = TRACKER_STATE.lock(); + let allocations = &mut tracker_state.allocations; + + // Print warning if we're missing allocations. + allocations.warn_on_problems(peak); + let allocated_bytes = if peak { + allocations.get_peak_allocated_bytes() + } else { + allocations.get_current_allocated_bytes() + }; + let flamegraph_callstacks = allocations.combine_callstacks(peak); + (allocated_bytes, flamegraph_callstacks) + }; + + eprintln!("=fil-profile= Preparing to write to {}", path); + let directory_path = Path::new(path); + + let title = format!( + "{} ({:.1} MiB)", + title, + allocated_bytes as f64 / (1024.0 * 1024.0) + ); + let subtitle = r#"Made with the Fil profiler. Try it on your code!"#; + flamegraph_callstacks.write_flamegraphs( + directory_path, + base_filename, + &title, + subtitle, + "bytes", + to_be_post_processed, + ) +} + /// Dump all callstacks in peak memory usage to format used by flamegraph. fn dump_peak_to_flamegraph(path: &str) { - let mut tracker_state = TRACKER_STATE.lock(); - let allocations = &mut tracker_state.allocations; - allocations.dump_peak_to_flamegraph(path); + dump_to_flamegraph(path, true, "peak-memory", "Peak Tracked Memory Usage", true); } #[no_mangle] diff --git a/memapi/src/flamegraph.rs b/memapi/src/flamegraph.rs index 10e775f..0e69888 100644 --- a/memapi/src/flamegraph.rs +++ b/memapi/src/flamegraph.rs @@ -66,7 +66,7 @@ pub fn write_lines>(lines: I, path: &Path) -> std /// A strategy for cleaning up callstacks before rendering them to text. pub trait CallstackCleaner { - fn cleanup(&self, callstack: &Callstack) -> Cow; + fn cleanup<'a>(&self, callstack: &'a Callstack) -> Cow<'a, Callstack>; } /// The data needed to create a flamegraph. @@ -99,7 +99,7 @@ where by_call.map(move |(callstack, size)| { format!( "{} {}", - (self.update_callstack)(callstack).as_string( + self.callstack_cleaner.cleanup(callstack).as_string( to_be_post_processed, &self.functions, ";", @@ -112,7 +112,7 @@ where /// Low-level interface for writing flamegraphs with post-processing: pub fn get_flamegraph_with_options( - &self, + &'a self, to_be_post_processed: bool, mut options: flamegraph::Options, // special cased because it needs to be psot-processed: @@ -147,7 +147,7 @@ where /// Write a flamegraph SVG to disk, given lines in summarized format. pub fn get_flamegraph( - &self, + &'a self, reversed: bool, title: &str, subtitle: &str, @@ -177,7 +177,7 @@ where /// Write a flamegraph SVG to disk. pub fn write_flamegraph( - &self, + &'a self, path: &Path, reversed: bool, title: &str, @@ -194,7 +194,7 @@ where /// Write .prof, -source.prof, .svg and -reversed.svg files for given lines. pub fn write_flamegraphs( - &self, + &'a self, directory_path: &Path, base_filename: &str, title: &str, diff --git a/memapi/src/memorytracking.rs b/memapi/src/memorytracking.rs index a80dea8..5fdca15 100644 --- a/memapi/src/memorytracking.rs +++ b/memapi/src/memorytracking.rs @@ -1,5 +1,4 @@ use crate::flamegraph::filter_to_useful_callstacks; -use crate::flamegraph::write_flamegraphs; use crate::flamegraph::CallstackCleaner; use crate::flamegraph::FlamegraphCallstacks; use crate::linecache::LineCacher; @@ -15,7 +14,6 @@ use serde::Serialize; use std::borrow::Cow; use std::collections::BTreeMap; use std::collections::HashMap; -use std::path::Path; extern "C" { fn _exit(exit_code: std::os::raw::c_int); @@ -371,10 +369,10 @@ impl Allocation { } /// A CallstackCleaner that leaves the callstack unchanged. -struct IdentityCleaner; +pub struct IdentityCleaner; impl CallstackCleaner for IdentityCleaner { - fn cleanup(&self, callstack: &Callstack) -> Cow { + fn cleanup<'a>(&self, callstack: &'a Callstack) -> Cow<'a, Callstack> { Cow::Borrowed(callstack) } } @@ -400,7 +398,7 @@ pub struct AllocationTracker { current_allocated_bytes: usize, peak_allocated_bytes: usize, // Default directory to write out data lacking other info: - default_path: String, + pub default_path: String, // Allocations that somehow disappeared. Not relevant for sampling profiler. missing_allocated_bytes: usize, @@ -616,7 +614,7 @@ impl AllocationTracker { /// Combine Callstacks and make them human-readable. Duplicate callstacks /// have their allocated memory summed. - fn combine_callstacks( + pub fn combine_callstacks( &mut self, // If false, will do the current allocations: peak: bool, @@ -661,24 +659,6 @@ impl AllocationTracker { self.peak_memory_usage.clear(); } - /// Dump information about where we are. - pub fn oom_dump(&mut self) { - eprintln!( - "=fil-profile= We'll try to dump out SVGs. Note that no HTML file will be written." - ); - let default_path = self.default_path.clone(); - self.dump_to_flamegraph( - &default_path, - false, - "out-of-memory", - "Current allocations at out-of-memory time", - false, - ); - unsafe { - _exit(53); - } - } - /// Validate internal state is in a good state. This won't pass until /// check_if_new_peak() is called. fn validate(&self) { @@ -704,6 +684,22 @@ impl AllocationTracker { assert!(self.peak_memory_usage.iter().sum::() == self.peak_allocated_bytes); } + /// Warn of untracked allocations; only relevant if you are profiling _all_ + /// allocations, i.e. Fil but not Sciagraph. + pub fn warn_on_problems(&self, peak: bool) { + let allocated_bytes = if peak { + self.get_peak_allocated_bytes() + } else { + self.get_current_allocated_bytes() + }; + if self.missing_allocated_bytes > 0 { + eprintln!("=fil-profile= WARNING: {:.2}% ({} bytes) of tracked memory somehow disappeared. If this is a small percentage you can just ignore this warning, since the missing allocations won't impact the profiling results. If the % is high, please run `export FIL_DEBUG=1` to get more output', re-run Fil on your script, and then file a bug report at https://github.com/pythonspeed/filprofiler/issues/new", self.missing_allocated_bytes as f64 * 100.0 / allocated_bytes as f64, self.missing_allocated_bytes); + } + if self.failed_deallocations > 0 { + eprintln!("=fil-profile= WARNING: Encountered {} deallocations of untracked allocations. A certain number are expected in normal operation, of allocations created before Fil started tracking, and even more if you're using the Fil API to turn tracking on and off.", self.failed_deallocations); + } + } + /// Reset internal state in way that doesn't invalidate e.g. thread-local /// caching of callstack ID. pub fn reset(&mut self, default_path: String) { From 923820749b0bbe1921af25a803c5294fb5dc42f1 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 17 Jan 2023 13:59:40 -0500 Subject: [PATCH 05/21] Switch to Python linecache. --- memapi/src/linecache.rs | 204 +++++++---------------------------- memapi/src/memorytracking.rs | 4 +- memapi/src/python.rs | 14 ++- 3 files changed, 53 insertions(+), 169 deletions(-) diff --git a/memapi/src/linecache.rs b/memapi/src/linecache.rs index 4ffe9f2..dbcf744 100644 --- a/memapi/src/linecache.rs +++ b/memapi/src/linecache.rs @@ -1,193 +1,65 @@ //! A line caching library, a bit like Python's linecache. +//! +//! We don't want to use Python's linecache because calling back into Python for +//! that can result in deadlocks. +use crate::python; use ahash::RandomState; -use parking_lot::Mutex; -use pyo3::{prelude::*, types::PyString}; -use std::{ - collections::HashMap, - fs::File, - io::{BufRead, BufReader, Cursor, Read}, - sync::Arc, -}; +use std::collections::HashMap; -/// Store a cache of files in memory, allow easy reading of lines. +/// Wrapper around Python's linecache, with extra caching so we can call Python +/// less. #[derive(Default)] pub struct LineCacher { - file_lines: HashMap, RandomState>, + linecache: HashMap<(String, usize), String, RandomState>, } -static EMPTY_STRING: String = String::new(); - impl LineCacher { - /// Add lines for a particular filename, if it's not already added, returning an Entry. - fn maybe_add_file_data<'a, GR: FnOnce() -> Option>>( - &'a mut self, - filename: &str, - get_data: GR, - ) -> &'a Vec { - self.file_lines - .entry(filename.to_string()) - .or_insert_with(|| { - get_data() - .map(|r| { - BufReader::new(r) - .lines() - .map(|l| l.unwrap_or_else(|_| EMPTY_STRING.clone())) - .collect() - }) - .unwrap_or_else(Vec::new) - }) - } - - /// Add a string-of-lines for a particular filename. - fn add_string(&mut self, filename: &str, lines: &str) { - let lines = Cursor::new(lines.to_string().into_bytes()); - self.maybe_add_file_data(filename, || Some(Box::new(lines))); - } - - /// Get the source code line for the given file. If the file doesn't exist - /// or line number is too big, an empty string is returned. Line endings are - /// stripped. - pub fn get_source_line<'a>(&'a mut self, filename: &str, line_number: usize) -> &'a str { + /// Get the source code line for the given file. + pub fn get_source_line(&mut self, filename: &str, line_number: usize) -> String { if line_number == 0 { - return &EMPTY_STRING; - } - let entry = self.maybe_add_file_data(filename, || { - File::open(filename).ok().map(|f| { - let b: Box = Box::new(f); - b - }) - }); - entry.get(line_number - 1).unwrap_or(&EMPTY_STRING) - } -} - -/// Wrapper around Python's linecache.cache dictionary that captures manual -/// writes. -/// -/// In particular, IPython/Jupyter cells will get added there, and so we want to -/// make sure we have access to them in our linecache so we can create correct -/// tracebacks. -#[pyclass] -struct PyLineCacheWrapper { - py_linecache: PyObject, - rust_linecache: Arc>, -} - -impl PyLineCacheWrapper { - fn new(py_linecache: &PyAny) -> Self { - // TODO populate Rust linecache with non-filesystem cached data. - // TODO and write test. - Self { - py_linecache: py_linecache.into(), - rust_linecache: Arc::new(Mutex::new(LineCacher::default())), + return String::new(); } + let entry = self + .linecache + .entry((filename.to_string(), line_number)) + .or_insert_with(|| { + python::get_source_line(&filename, line_number).unwrap_or(String::new()) + }); + entry.clone() } } -#[pymethods] -impl PyLineCacheWrapper { - /// Pass through pretty much everything to the underyling dict: - fn __getattribute__(slf: PyRef<'_, Self>, attr: &PyString) -> PyResult> { - slf.py_linecache.getattr(slf.py(), attr) - } - - /// __setitem__ is only code we handle specially: - fn __setitem__(slf: PyRef<'_, Self>, attr: &PyAny, value: &PyAny) -> PyResult<()> { - // TODO if entry is not a file, added it to rust cache. - // TODO and test. - println!("ADDING {} to linecache", attr); - let py = slf.py(); - slf.py_linecache - .getattr(py, "__setitem__")? - .call(py, (attr, value), None)?; - Ok(()) - } -} - -/// Make sure changes to the Python linecache end up in our cache, in particular -/// for Jupyter which manually adds item to Python's cache. -fn monkeypatch_python_linecache() -> PyResult<()> { - // TODO write test showing this changes the global linecache. - Python::with_gil(|py| { - let linecache = py.import("linecache")?; - let py_cache = linecache.getattr("cache")?; - let wrapper = PyLineCacheWrapper::new(py_cache); - linecache.setattr("cache", Py::new(py, wrapper)?)?; - Ok(()) - }) -} - -/// Get access to the LineCacher that is attached to `linecache` Python module -/// after calling `monkeypatch_python_linecache()`. -/// -/// If none is installed, a new one will be installed. -/// -/// If it's impossible to install for some reason, a temporary one-off will be -/// created, since at least filesystem tracebacks can still be used. -pub fn run_with_linecache(closure: F) { - // TODO write test showing this gets existing object attached to global linecache. - // TODO write test showing this attaches new one if global linecache is missing this object. - fn get_wrapper(py: Python<'_>) -> PyResult>> { - let linecache = py.import("linecache")?; - let wrapper = linecache.getattr("cache")?; - let wrapper: &PyCell = wrapper.downcast()?; - Ok(wrapper.try_borrow()?.rust_linecache.clone()) - } - - let linecacher = Python::with_gil(|py| { - get_wrapper(py) - .or_else(|_| { - monkeypatch_python_linecache(); - get_wrapper(py) - }) - .unwrap_or_else(|_| Arc::new(Mutex::new(LineCacher::default()))) - }); - closure(&linecacher.lock()); -} - #[cfg(test)] mod tests { use super::*; use rusty_fork::rusty_fork_test; use std::io::Write; - #[test] - fn linecacher() { - let mut cache = LineCacher::default(); - - // Non-existent file - assert_eq!(cache.get_source_line("/no/such/file", 1), ""); + rusty_fork_test! { + /// The linecache can read files. + #[test] + fn linecacher_from_file() { + pyo3::prepare_freethreaded_python(); + let mut cache = LineCacher::default(); - let mut f = tempfile::NamedTempFile::new().unwrap(); - f.as_file_mut().write_all(b"abc\ndef\r\nghijk").unwrap(); - let path = f.path().as_os_str().to_str().unwrap(); + // Non-existent file + assert_eq!(cache.get_source_line("/no/such/file", 1), ""); - // 0 line number - assert_eq!(cache.get_source_line(path, 0), ""); + let mut f = tempfile::NamedTempFile::new().unwrap(); + f.as_file_mut().write_all(b"abc\ndef\r\nghijk").unwrap(); + let path = f.path().as_os_str().to_str().unwrap(); - // Too high line number - assert_eq!(cache.get_source_line(path, 4), ""); + // 0 line number + assert_eq!(cache.get_source_line(path, 0), ""); - // Present line numbers - assert_eq!(cache.get_source_line(path, 1), "abc"); - assert_eq!(cache.get_source_line(path, 2), "def"); - assert_eq!(cache.get_source_line(path, 3), "ghijk"); - } + // Too high line number + assert_eq!(cache.get_source_line(path, 4), ""); - #[test] - fn linecacher_add_string() { - let mut cache = LineCacher::default(); - cache.add_string("file1", "a\nb"); - cache.add_string("file2", "c\nd"); - assert_eq!(cache.get_source_line("file1", 1), "a"); - assert_eq!(cache.get_source_line("file1", 2), "b"); - assert_eq!(cache.get_source_line("file2", 1), "c"); - assert_eq!(cache.get_source_line("file2", 2), "d"); - } - - rusty_fork_test! { - //#[test] - //fn + // Present line numbers + assert_eq!(cache.get_source_line(path, 1), "abc"); + assert_eq!(cache.get_source_line(path, 2), "def"); + assert_eq!(cache.get_source_line(path, 3), "ghijk"); + } } } diff --git a/memapi/src/memorytracking.rs b/memapi/src/memorytracking.rs index 5fdca15..08d14b0 100644 --- a/memapi/src/memorytracking.rs +++ b/memapi/src/memorytracking.rs @@ -236,12 +236,12 @@ impl Callstack { // which I can't be bothered to fix right now, so for // now do hack where we shove in some other character // that can be fixed in post-processing. - let code = code.replace(" ", "\u{12e4}"); + let code = code.replace(' ', "\u{12e4}"); // Semicolons are used as separator in the flamegraph // input format, so need to replace them with some other // character. We use "full-width semicolon", and then // replace it back in post-processing. - let code = code.replace(";", "\u{ff1b}"); + let code = code.replace(';', "\u{ff1b}"); // The \u{2800} is to ensure we don't have empty lines, // and that whitespace doesn't get trimmed from start; // we'll get rid of this in post-processing. diff --git a/memapi/src/python.rs b/memapi/src/python.rs index 610e53a..61cc395 100644 --- a/memapi/src/python.rs +++ b/memapi/src/python.rs @@ -1,7 +1,19 @@ //! Interactions with Python APIs. use once_cell::sync::Lazy; -use pyo3::{types::PyModule, Python}; +use pyo3::prelude::*; + +// Get the source code line from a given filename. +pub fn get_source_line(filename: &str, line_number: usize) -> PyResult { + Python::with_gil(|py| { + let linecache = PyModule::import(py, "linecache")?; + let result: String = linecache + .getattr("getline")? + .call1((filename, line_number))? + .to_string(); + Ok(result) + }) +} // Return the filesystem path of the stdlib's runpy module. pub fn get_runpy_path() -> &'static str { From 816c9f04ffb316e70d0533c969ca4781a128b9d7 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 17 Jan 2023 14:46:02 -0500 Subject: [PATCH 06/21] Lints. --- filpreload/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/filpreload/src/lib.rs b/filpreload/src/lib.rs index d42fcbf..e962df7 100644 --- a/filpreload/src/lib.rs +++ b/filpreload/src/lib.rs @@ -379,8 +379,8 @@ extern "C" { fn is_initialized() -> c_int; // Increment/decrement reentrancy counter. - fn fil_increment_reentrancy(); - fn fil_decrement_reentrancy(); + //fn fil_increment_reentrancy(); + //fn fil_decrement_reentrancy(); } struct FilMmapAPI; @@ -398,7 +398,7 @@ impl pymemprofile_api::mmap::MmapAPI for FilMmapAPI { } fn is_initialized(&self) -> bool { - return unsafe { is_initialized() == 1 }; + unsafe { is_initialized() == 1 } } } From a0d7ee48163cc383236ca3711ebae16da2b061de Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 17 Jan 2023 14:46:21 -0500 Subject: [PATCH 07/21] Unit testing, give up on Rust caching for now. --- memapi/src/linecache.rs | 42 +++++++++++++++++++++--------------- memapi/src/memorytracking.rs | 11 +++++++--- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/memapi/src/linecache.rs b/memapi/src/linecache.rs index dbcf744..de538c0 100644 --- a/memapi/src/linecache.rs +++ b/memapi/src/linecache.rs @@ -4,15 +4,10 @@ //! that can result in deadlocks. use crate::python; -use ahash::RandomState; -use std::collections::HashMap; -/// Wrapper around Python's linecache, with extra caching so we can call Python -/// less. +/// Wrapper around Python's linecache. #[derive(Default)] -pub struct LineCacher { - linecache: HashMap<(String, usize), String, RandomState>, -} +pub struct LineCacher {} impl LineCacher { /// Get the source code line for the given file. @@ -20,19 +15,14 @@ impl LineCacher { if line_number == 0 { return String::new(); } - let entry = self - .linecache - .entry((filename.to_string(), line_number)) - .or_insert_with(|| { - python::get_source_line(&filename, line_number).unwrap_or(String::new()) - }); - entry.clone() + python::get_source_line(filename, line_number).unwrap_or_default() } } #[cfg(test)] mod tests { use super::*; + use pyo3::prelude::*; use rusty_fork::rusty_fork_test; use std::io::Write; @@ -57,9 +47,27 @@ mod tests { assert_eq!(cache.get_source_line(path, 4), ""); // Present line numbers - assert_eq!(cache.get_source_line(path, 1), "abc"); - assert_eq!(cache.get_source_line(path, 2), "def"); - assert_eq!(cache.get_source_line(path, 3), "ghijk"); + assert_eq!(cache.get_source_line(path, 1), "abc\n"); + assert_eq!(cache.get_source_line(path, 2), "def\n"); + assert_eq!(cache.get_source_line(path, 3), "ghijk\n"); + } + + /// The linecache can read random crap shoved into the linecache module. + #[test] + fn linecacher_from_arbitrary_source() { + pyo3::prepare_freethreaded_python(); + let mut cache = LineCacher::default(); + + Python::with_gil(|py| { + let blah = vec!["arr\n", "boo"]; + let linecache = PyModule::import(py, "linecache")?; + linecache + .getattr("cache")?.set_item("blah", (8, 0, blah, "blah"))?; + Ok::<(), PyErr>(()) + }).unwrap(); + + assert_eq!(cache.get_source_line("blah", 1), "arr\n"); + assert_eq!(cache.get_source_line("blah", 2), "boo"); } } } diff --git a/memapi/src/memorytracking.rs b/memapi/src/memorytracking.rs index 08d14b0..e32fb29 100644 --- a/memapi/src/memorytracking.rs +++ b/memapi/src/memorytracking.rs @@ -236,7 +236,12 @@ impl Callstack { // which I can't be bothered to fix right now, so for // now do hack where we shove in some other character // that can be fixed in post-processing. - let code = code.replace(' ', "\u{12e4}"); + let code = code.trim_end().replace(' ', "\u{12e4}"); + // Tabs == 8 spaces in Python. + let code = code.replace( + '\t', + "\u{12e4}\u{12e4}\u{12e4}\u{12e4}\u{12e4}\u{12e4}\u{12e4}\u{12e4}", + ); // Semicolons are used as separator in the flamegraph // input format, so need to replace them with some other // character. We use "full-width semicolon", and then @@ -718,7 +723,7 @@ impl AllocationTracker { #[cfg(test)] mod tests { - use crate::memorytracking::{same_callstack, ProcessUid, PARENT_PROCESS}; + use crate::memorytracking::{ProcessUid, PARENT_PROCESS}; use super::LineNumberInfo::LineNumber; use super::{ @@ -1178,7 +1183,7 @@ mod tests { "c:3 (cf) 234", "a:7 (af);b:2 (bf) 6000", ]; - let mut result2: Vec = tracker.to_lines(true, false, same_callstack).collect(); + let mut result2: Vec = tracker.combine_callstacks(true).to_lines(false).collect(); result2.sort(); expected2.sort(); assert_eq!(expected2, result2); From b640a8a3379bff9bfe92c70079ccc776420bc908 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 17 Jan 2023 14:55:00 -0500 Subject: [PATCH 08/21] Better Sciagraph ad. --- filprofiler/_report.py | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/filprofiler/_report.py b/filprofiler/_report.py index 308151d..90a5bdd 100644 --- a/filprofiler/_report.py +++ b/filprofiler/_report.py @@ -76,19 +76,16 @@ def render_report(output_path: str, now: datetime) -> str:

Profiling result


-

Check out my other project:

-

Find memory and performance bottlenecks in production!

-

When your data pipeline is too slow in production, reproducing the problem - on your laptop is hard or impossible—which means identifying and fixing the problem can be tricky.

-

What if you knew the cause of the problem as soon as you realized it was happening?

-

That's how - the Sciagraph profiler can help you: - it's designed to find performance - and memory bottlenecks by continuously profiling in production.

+

Find performance bottlenecks in your data processing jobs with the Sciagraph profiler

+

The Sciagraph profiler can help you + find performance + and memory bottlenecks with low overhead, so you can use it in both development and production.

+

Unlike Fil, it includes performance profiling. Sciagraph's memory profiling uses sampling so it runs faster than Fil, but unlike Fil + it can't accurately profile small allocations or run natively on macOS.



· From bf528d56719a346b7e71f87d80a0c941de0a114a Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 17 Jan 2023 17:24:56 -0500 Subject: [PATCH 09/21] Jupyter test to ensure source code is included. --- tests/test_endtoend.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/test_endtoend.py b/tests/test_endtoend.py index 423c112..df4be6c 100644 --- a/tests/test_endtoend.py +++ b/tests/test_endtoend.py @@ -491,7 +491,16 @@ def test_jupyter(tmpdir): assert " Date: Tue, 17 Jan 2023 17:28:03 -0500 Subject: [PATCH 10/21] News fragment. --- .changelog/474.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 .changelog/474.bugfix diff --git a/.changelog/474.bugfix b/.changelog/474.bugfix new file mode 100644 index 0000000..eba62af --- /dev/null +++ b/.changelog/474.bugfix @@ -0,0 +1 @@ +Fix regression where source code would sometimes be missing from flamegraphs, most notably in Jupyter profiling. \ No newline at end of file From 9e9ba6b466fa8c89e4f9124a20b576d8dd6aa9f6 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 17 Jan 2023 20:04:22 -0500 Subject: [PATCH 11/21] Require ExactSizeIterator. --- memapi/src/flamegraph.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/memapi/src/flamegraph.rs b/memapi/src/flamegraph.rs index 0e69888..365973b 100644 --- a/memapi/src/flamegraph.rs +++ b/memapi/src/flamegraph.rs @@ -79,6 +79,7 @@ pub struct FlamegraphCallstacks { impl<'a, D, FL, UC> FlamegraphCallstacks where &'a D: IntoIterator, + <&'a D as IntoIterator>::IntoIter: ExactSizeIterator, D: 'a, FL: FunctionLocations, UC: CallstackCleaner, From afdc0fe933e336177ff1c941457914b90fada7be Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Tue, 17 Jan 2023 20:04:43 -0500 Subject: [PATCH 12/21] Fix lint. --- memapi/src/flamegraph.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/memapi/src/flamegraph.rs b/memapi/src/flamegraph.rs index 365973b..2da9792 100644 --- a/memapi/src/flamegraph.rs +++ b/memapi/src/flamegraph.rs @@ -285,7 +285,7 @@ mod tests { ) { let total_size : usize = allocated_sizes.iter().sum(); let total_size_99 = (99 * total_size) / 100; - let callstacks = (&allocated_sizes).iter().enumerate(); + let callstacks = allocated_sizes.iter().enumerate(); let filtered : HashMap = filter_to_useful_callstacks(callstacks, total_size).collect(); let filtered_size :usize = filtered.values().into_iter().sum(); if filtered_size >= total_size_99 { From 8335bfc8e32098d8491183756431704822681f27 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 18 Jan 2023 09:04:02 -0500 Subject: [PATCH 13/21] Make CallstackCleaner pluggable, for benefit of Sciagraph. --- filpreload/src/lib.rs | 5 +++-- memapi/src/memorytracking.rs | 14 +++++++++----- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/filpreload/src/lib.rs b/filpreload/src/lib.rs index e962df7..d92009e 100644 --- a/filpreload/src/lib.rs +++ b/filpreload/src/lib.rs @@ -2,7 +2,8 @@ use parking_lot::Mutex; use pymemprofile_api::memorytracking::LineNumberInfo::LineNumber; use pymemprofile_api::memorytracking::{ - AllocationTracker, CallSiteId, Callstack, FunctionId, VecFunctionLocations, PARENT_PROCESS, + AllocationTracker, CallSiteId, Callstack, FunctionId, IdentityCleaner, VecFunctionLocations, + PARENT_PROCESS, }; use pymemprofile_api::oom::{InfiniteMemory, OutOfMemoryEstimator, RealMemoryInfo}; use std::cell::RefCell; @@ -220,7 +221,7 @@ fn dump_to_flamegraph( } else { allocations.get_current_allocated_bytes() }; - let flamegraph_callstacks = allocations.combine_callstacks(peak); + let flamegraph_callstacks = allocations.combine_callstacks(peak, IdentityCleaner); (allocated_bytes, flamegraph_callstacks) }; diff --git a/memapi/src/memorytracking.rs b/memapi/src/memorytracking.rs index e32fb29..dbb1797 100644 --- a/memapi/src/memorytracking.rs +++ b/memapi/src/memorytracking.rs @@ -619,11 +619,12 @@ impl AllocationTracker { /// Combine Callstacks and make them human-readable. Duplicate callstacks /// have their allocated memory summed. - pub fn combine_callstacks( + pub fn combine_callstacks( &mut self, // If false, will do the current allocations: peak: bool, - ) -> FlamegraphCallstacks, FL, IdentityCleaner> { + callstack_cleaner: CC, + ) -> FlamegraphCallstacks, FL, CC> { // Would be nice to validate if data is consistent. However, there are // edge cases that make it slightly inconsistent (e.g. see the // unexpected code path in add_allocation() above), and blowing up @@ -654,7 +655,7 @@ impl AllocationTracker { }) .collect(), self.functions.cheap_clone(), - IdentityCleaner {}, + callstack_cleaner, ) } @@ -723,7 +724,7 @@ impl AllocationTracker { #[cfg(test)] mod tests { - use crate::memorytracking::{ProcessUid, PARENT_PROCESS}; + use crate::memorytracking::{IdentityCleaner, ProcessUid, PARENT_PROCESS}; use super::LineNumberInfo::LineNumber; use super::{ @@ -1183,7 +1184,10 @@ mod tests { "c:3 (cf) 234", "a:7 (af);b:2 (bf) 6000", ]; - let mut result2: Vec = tracker.combine_callstacks(true).to_lines(false).collect(); + let mut result2: Vec = tracker + .combine_callstacks(true, IdentityCleaner) + .to_lines(false) + .collect(); result2.sort(); expected2.sort(); assert_eq!(expected2, result2); From 647a320282217dd2acd6cfe47122222410070eeb Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 18 Jan 2023 09:09:34 -0500 Subject: [PATCH 14/21] Update pyo3. --- Cargo.lock | 33 +++++++++++++++++++++------------ filpreload/Cargo.toml | 2 +- memapi/Cargo.toml | 2 +- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3b69e99..519007e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -545,6 +545,15 @@ dependencies = [ "autocfg", ] +[[package]] +name = "memoffset" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d61c719bcfbcf5d62b3a09efa6088de8c54bc0bfcd3ea7ae39fcc186108b8de1" +dependencies = [ + "autocfg", +] + [[package]] name = "minimal-lexical" version = "0.2.1" @@ -570,7 +579,7 @@ dependencies = [ "cc", "cfg-if", "libc", - "memoffset", + "memoffset 0.6.5", ] [[package]] @@ -753,14 +762,14 @@ dependencies = [ [[package]] name = "pyo3" -version = "0.17.3" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "268be0c73583c183f2b14052337465768c07726936a260f480f0857cb95ba543" +checksum = "ccd4149c8c3975099622b4e1962dac27565cf5663b76452c3e2b66e0b6824277" dependencies = [ "cfg-if", "indoc", "libc", - "memoffset", + "memoffset 0.8.0", "parking_lot", "pyo3-build-config", "pyo3-ffi", @@ -770,9 +779,9 @@ dependencies = [ [[package]] name = "pyo3-build-config" -version = "0.17.3" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "28fcd1e73f06ec85bf3280c48c67e731d8290ad3d730f8be9dc07946923005c8" +checksum = "9cd09fe469834db21ee60e0051030339e5d361293d8cb5ec02facf7fdcf52dbf" dependencies = [ "once_cell", "target-lexicon", @@ -780,9 +789,9 @@ dependencies = [ [[package]] name = "pyo3-ffi" -version = "0.17.3" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0f6cb136e222e49115b3c51c32792886defbfb0adead26a688142b346a0b9ffc" +checksum = "0c427c9a96b9c5b12156dbc11f76b14f49e9aae8905ca783ea87c249044ef137" dependencies = [ "libc", "pyo3-build-config", @@ -790,9 +799,9 @@ dependencies = [ [[package]] name = "pyo3-macros" -version = "0.17.3" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "94144a1266e236b1c932682136dc35a9dee8d3589728f68130c7c3861ef96b28" +checksum = "16b822bbba9d60630a44d2109bc410489bb2f439b33e3a14ddeb8a40b378a7c4" dependencies = [ "proc-macro2", "pyo3-macros-backend", @@ -802,9 +811,9 @@ dependencies = [ [[package]] name = "pyo3-macros-backend" -version = "0.17.3" +version = "0.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c8df9be978a2d2f0cdebabb03206ed73b11314701a5bfe71b0d753b81997777f" +checksum = "84ae898104f7c99db06231160770f3e40dad6eb9021daddc0fedfa3e41dff10a" dependencies = [ "proc-macro2", "quote", diff --git a/filpreload/Cargo.toml b/filpreload/Cargo.toml index bd9c113..93fb629 100644 --- a/filpreload/Cargo.toml +++ b/filpreload/Cargo.toml @@ -18,7 +18,7 @@ path = "../memapi" features = [] [dependencies.pyo3] -version = "0.17" +version = "0.18" default-features = false [build-dependencies] diff --git a/memapi/Cargo.toml b/memapi/Cargo.toml index e19a0d7..a94c455 100644 --- a/memapi/Cargo.toml +++ b/memapi/Cargo.toml @@ -30,7 +30,7 @@ default-features = false features = ["memory", "process"] [dependencies.pyo3] -version = "0.17" +version = "0.18" [target.'cfg(target_os = "linux")'.dependencies] cgroups-rs = "0.2.11" From dbdbf1c9b9107030f125247f2d1911fb8e017753 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 18 Jan 2023 09:48:27 -0500 Subject: [PATCH 15/21] Fix the test. --- tests/test_endtoend.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_endtoend.py b/tests/test_endtoend.py index df4be6c..d501107 100644 --- a/tests/test_endtoend.py +++ b/tests/test_endtoend.py @@ -618,7 +618,10 @@ def test_tabs(): svg = f.read() # Tabs are still there: - assert ">\tarr1, arr2 = make_".replace(" ", "\u00a0") in svg + assert ( + ">\tarr1, arr2 = make_".replace(" ", "\u00a0").replace("\t", "\u00a0" * 8) + in svg + ) # It's valid XML: ElementTree.fromstring(svg) From 4fec0b374e24cd4abe143de17b025f1dd7a7d449 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 18 Jan 2023 19:04:41 -0500 Subject: [PATCH 16/21] Should be ExactSizeIterator. --- memapi/src/flamegraph.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/memapi/src/flamegraph.rs b/memapi/src/flamegraph.rs index 2da9792..2aa5af5 100644 --- a/memapi/src/flamegraph.rs +++ b/memapi/src/flamegraph.rs @@ -94,7 +94,10 @@ where /// Create iterator over the line-based string format parsed by the inferno /// crate. - pub fn to_lines(&'a self, to_be_post_processed: bool) -> impl Iterator + 'a { + pub fn to_lines( + &'a self, + to_be_post_processed: bool, + ) -> impl ExactSizeIterator + 'a { let by_call = (&self.data).into_iter(); let mut linecache = LineCacher::default(); by_call.map(move |(callstack, size)| { From 0b96f710ce4e8857bc3eb3c78a377b61968a5d8f Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 18 Jan 2023 19:08:45 -0500 Subject: [PATCH 17/21] Original way we had of referring to this. --- filpreload/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/filpreload/src/lib.rs b/filpreload/src/lib.rs index d92009e..77d733d 100644 --- a/filpreload/src/lib.rs +++ b/filpreload/src/lib.rs @@ -89,6 +89,7 @@ fn set_current_callstack(callstack: &Callstack) { } extern "C" { + fn _exit(exit_code: std::os::raw::c_int); fn free(address: *mut c_void); } @@ -169,7 +170,7 @@ fn add_allocation( false, ); unsafe { - libc::_exit(53); + _exit(53); } }; Ok(()) From c2adc0a9c1750e4f596f8af4f968c79b5f1912b8 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 19 Jan 2023 15:21:45 -0500 Subject: [PATCH 18/21] Make the functions public again. --- memapi/src/flamegraph.rs | 118 ++++++++++++++++++++++++--------------- 1 file changed, 74 insertions(+), 44 deletions(-) diff --git a/memapi/src/flamegraph.rs b/memapi/src/flamegraph.rs index 2aa5af5..ad93cbf 100644 --- a/memapi/src/flamegraph.rs +++ b/memapi/src/flamegraph.rs @@ -118,35 +118,12 @@ where pub fn get_flamegraph_with_options( &'a self, to_be_post_processed: bool, - mut options: flamegraph::Options, + options: flamegraph::Options, // special cased because it needs to be psot-processed: subtitle: Option<&str>, ) -> Result, Box> { let lines = self.to_lines(to_be_post_processed); - let mut output = vec![]; - let lines: Vec = lines.into_iter().collect(); - match flamegraph::from_lines(&mut options, lines.iter().map(|s| s.as_ref()), &mut output) { - Err(e) => Err(format!("{}", e).into()), - Ok(_) => { - if to_be_post_processed { - // Replace with real subtitle. - let data = String::from_utf8(output)?; - let data = if let Some(subtitle) = subtitle { - data.replace("__FIL-SUBTITLE-HERE__", subtitle) - } else { - data - }; - // Restore normal semi-colons. - let data = data.replace('\u{ff1b}', ";"); - // Restore (non-breaking) spaces. - let data = data.replace('\u{12e4}', "\u{00a0}"); - // Get rid of empty-line markers: - let data = data.replace('\u{2800}', ""); - output = data.as_bytes().to_vec(); - } - Ok(output) - } - } + get_flamegraph_with_options(lines, to_be_post_processed, options, subtitle) } /// Write a flamegraph SVG to disk, given lines in summarized format. @@ -158,25 +135,14 @@ where count_name: &str, to_be_post_processed: bool, ) -> Result, Box> { - let title = format!("{}{}", title, if reversed { ", Reversed" } else { "" },); - let mut options = flamegraph::Options::default(); - options.title = title; - options.count_name = count_name.to_string(); - options.font_size = 16; - options.font_type = "monospace".to_string(); - options.frame_height = 22; - options.reverse_stack_order = reversed; - options.color_diffusion = true; - options.direction = flamegraph::Direction::Inverted; - options.min_width = 0.2; - // Maybe disable this some day; but for now it makes debugging much - // easier: - options.pretty_xml = true; - if to_be_post_processed { - // Can't put structured text into subtitle, so have to do a hack. - options.subtitle = Some("__FIL-SUBTITLE-HERE__".to_string()); - } - self.get_flamegraph_with_options(to_be_post_processed, options, Some(subtitle)) + get_flamegraph( + self.to_lines(to_be_post_processed), + reversed, + title, + subtitle, + count_name, + to_be_post_processed, + ) } /// Write a flamegraph SVG to disk. @@ -273,6 +239,70 @@ where } } +/// Low-level interface for writing flamegraphs with post-processing: +pub fn get_flamegraph_with_options>( + lines: I, + to_be_post_processed: bool, + mut options: flamegraph::Options, + // special cased because it needs to be psot-processed: + subtitle: Option<&str>, +) -> Result, Box> { + let mut output = vec![]; + let lines: Vec = lines.into_iter().collect(); + match flamegraph::from_lines(&mut options, lines.iter().map(|s| s.as_ref()), &mut output) { + Err(e) => Err(format!("{}", e).into()), + Ok(_) => { + if to_be_post_processed { + // Replace with real subtitle. + let data = String::from_utf8(output)?; + let data = if let Some(subtitle) = subtitle { + data.replace("__FIL-SUBTITLE-HERE__", subtitle) + } else { + data + }; + // Restore normal semi-colons. + let data = data.replace('\u{ff1b}', ";"); + // Restore (non-breaking) spaces. + let data = data.replace('\u{12e4}', "\u{00a0}"); + // Get rid of empty-line markers: + let data = data.replace('\u{2800}', ""); + output = data.as_bytes().to_vec(); + } + Ok(output) + } + } +} + +/// Write a flamegraph SVG to disk, given lines in summarized format. +pub fn get_flamegraph>( + lines: I, + reversed: bool, + title: &str, + subtitle: &str, + count_name: &str, + to_be_post_processed: bool, +) -> Result, Box> { + let title = format!("{}{}", title, if reversed { ", Reversed" } else { "" },); + let mut options = flamegraph::Options::default(); + options.title = title; + options.count_name = count_name.to_string(); + options.font_size = 16; + options.font_type = "monospace".to_string(); + options.frame_height = 22; + options.reverse_stack_order = reversed; + options.color_diffusion = true; + options.direction = flamegraph::Direction::Inverted; + options.min_width = 0.2; + // Maybe disable this some day; but for now it makes debugging much + // easier: + options.pretty_xml = true; + if to_be_post_processed { + // Can't put structured text into subtitle, so have to do a hack. + options.subtitle = Some("__FIL-SUBTITLE-HERE__".to_string()); + } + get_flamegraph_with_options(lines, to_be_post_processed, options, Some(subtitle)) +} + #[cfg(test)] mod tests { use super::filter_to_useful_callstacks; From 29659397c084a9fea5c7b363563b59e03ae25901 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 19 Jan 2023 19:38:36 -0500 Subject: [PATCH 19/21] Switch to immutable data structure. --- memapi/src/memorytracking.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/memapi/src/memorytracking.rs b/memapi/src/memorytracking.rs index dbb1797..0bf41b4 100644 --- a/memapi/src/memorytracking.rs +++ b/memapi/src/memorytracking.rs @@ -53,21 +53,20 @@ pub trait FunctionLocations { /// Stores FunctionLocations, returns a FunctionId #[derive(Clone)] pub struct VecFunctionLocations { - // TODO switch to immutable vec - functions: Vec, + functions: ImVector, } impl VecFunctionLocations { /// Create a new tracker. pub fn new() -> Self { Self { - functions: Vec::with_capacity(8192), + functions: ImVector::new(), } } /// Register a function, get back its id. pub fn add_function(&mut self, filename: String, function_name: String) -> FunctionId { - self.functions.push(FunctionLocation { + self.functions.push_back(FunctionLocation { filename, function_name, }); From 4bbd390c4486dac5f8b0041864f7fdd1a85a7ad7 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 19 Jan 2023 19:43:12 -0500 Subject: [PATCH 20/21] More correct comment. --- memapi/src/linecache.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/memapi/src/linecache.rs b/memapi/src/linecache.rs index de538c0..5ddfe23 100644 --- a/memapi/src/linecache.rs +++ b/memapi/src/linecache.rs @@ -1,7 +1,6 @@ -//! A line caching library, a bit like Python's linecache. -//! -//! We don't want to use Python's linecache because calling back into Python for -//! that can result in deadlocks. +//! A Rust wrapper around Python's linecache. We can't just emulate it because +//! PEP 302 `__loader__`s and ipython shoving stuff into it and oh god oh god oh +//! god Python is complicated. use crate::python; From 39b700c58e5802662079ebfebcedcc83109dd6ac Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Fri, 20 Jan 2023 10:03:09 -0500 Subject: [PATCH 21/21] Prep for release. --- .changelog/474.bugfix | 1 - CHANGELOG.md | 6 ++++++ 2 files changed, 6 insertions(+), 1 deletion(-) delete mode 100644 .changelog/474.bugfix diff --git a/.changelog/474.bugfix b/.changelog/474.bugfix deleted file mode 100644 index eba62af..0000000 --- a/.changelog/474.bugfix +++ /dev/null @@ -1 +0,0 @@ -Fix regression where source code would sometimes be missing from flamegraphs, most notably in Jupyter profiling. \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index d9d30c6..73522e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Release notes +## 2023.1.0 (2023-1-20) + +### Bugfixes + +- Fix regression where source code would sometimes be missing from flamegraphs, most notably in Jupyter profiling. ([#474](https://github.com/pythonspeed/filprofiler/issues/474)) + ## 2022.11.0 (2022-11-07) ### Features