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

Implement --unstable-presymbolicate #202

Merged
merged 10 commits into from
May 21, 2024
5 changes: 4 additions & 1 deletion fxprof-processed-profile/src/frame_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl FrameTable {
resource_table: &mut ResourceTable,
func_table: &mut FuncTable,
native_symbol_table: &mut NativeSymbols,
global_libs: &GlobalLibTable,
global_libs: &mut GlobalLibTable,
frame: InternalFrame,
) -> usize {
let addresses = &mut self.addresses;
Expand Down Expand Up @@ -73,6 +73,9 @@ impl FrameTable {
(Some(native_symbol), name_string_index)
}
None => {
// This isn't in the pre-provided symbol table, and we know it's in a library.
global_libs.add_lib_used_rva(lib_index, address);

let location_string = format!("0x{address:x}");
(None, string_table.index_for_string(&location_string))
}
Expand Down
45 changes: 44 additions & 1 deletion fxprof-processed-profile/src/global_lib_table.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::collections::BTreeSet;
use std::sync::Arc;

use serde::ser::{Serialize, Serializer};
Expand All @@ -14,6 +15,11 @@ pub struct GlobalLibTable {
used_libs: Vec<LibraryHandle>, // append-only for stable GlobalLibIndexes
lib_map: FastHashMap<LibraryInfo, LibraryHandle>,
used_lib_map: FastHashMap<LibraryHandle, GlobalLibIndex>,
/// We keep track of RVA addresses that exist in frames that are assigned to this
/// library, so that we can potentially provide symbolication info ahead of time.
/// This is here instead of in `LibraryInfo` because we don't want to serialize it,
/// and because it's currently a hack.
all_libs_seen_rvas: Vec<Option<BTreeSet<u32>>>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment saying that the Vec is indexed by LibraryHandle.0.

Alternatively, you could make the Vec indexed by GlobalLibIndex.0, needing one less translation in add_lib_used_rva and at that point you could also remove the Option and always initialize it to the empty set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good idea. FWIW, GlobalLibIndex is a confusing name, maybe just UsedLibraryHandle?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UsedLibraryHandle sounds good

}

impl GlobalLibTable {
Expand All @@ -23,6 +29,7 @@ impl GlobalLibTable {
used_libs: Vec::new(),
lib_map: FastHashMap::default(),
used_lib_map: FastHashMap::default(),
all_libs_seen_rvas: Vec::new(),
}
}

Expand All @@ -31,6 +38,7 @@ impl GlobalLibTable {
*self.lib_map.entry(lib.clone()).or_insert_with(|| {
let handle = LibraryHandle(all_libs.len());
all_libs.push(lib);
self.all_libs_seen_rvas.push(None);
handle
})
}
Expand All @@ -52,6 +60,19 @@ impl GlobalLibTable {
let handle = self.used_libs.get(index.0)?;
self.all_libs.get(handle.0)
}

pub fn add_lib_used_rva(&mut self, index: GlobalLibIndex, address: u32) {
let handle = self.used_libs.get(index.0).unwrap();
let lib_seen_rvas = self.all_libs_seen_rvas[handle.0].get_or_insert_with(BTreeSet::new);
lib_seen_rvas.insert(address);
}

pub fn lib_used_rva_iter(&self) -> UsedLibraryAddressesIterator {
UsedLibraryAddressesIterator {
next_used_lib_index: 0,
global_lib_table: self,
}
}
}

impl Serialize for GlobalLibTable {
Expand All @@ -73,4 +94,26 @@ impl Serialize for GlobalLibIndex {

/// The handle for a library, obtained from [`Profile::add_lib`](crate::Profile::add_lib).
#[derive(Debug, Clone, Copy, PartialOrd, Ord, PartialEq, Eq, Hash)]
pub struct LibraryHandle(usize);
pub struct LibraryHandle(pub(crate) usize);
vvuk marked this conversation as resolved.
Show resolved Hide resolved

pub struct UsedLibraryAddressesIterator<'a> {
next_used_lib_index: usize,
global_lib_table: &'a GlobalLibTable,
}

impl<'a> Iterator for UsedLibraryAddressesIterator<'a> {
type Item = (&'a LibraryInfo, Option<&'a BTreeSet<u32>>);

fn next(&mut self) -> Option<Self::Item> {
self.global_lib_table
.used_libs
.get(self.next_used_lib_index)
vvuk marked this conversation as resolved.
Show resolved Hide resolved
.map(|lib| {
self.next_used_lib_index += 1;
(
&self.global_lib_table.all_libs[lib.0],
self.global_lib_table.all_libs_seen_rvas[lib.0].as_ref(),
)
})
}
}
2 changes: 1 addition & 1 deletion fxprof-processed-profile/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ pub use category_color::CategoryColor;
pub use counters::CounterHandle;
pub use cpu_delta::CpuDelta;
pub use frame::{Frame, FrameFlags, FrameInfo};
pub use global_lib_table::LibraryHandle;
pub use global_lib_table::{LibraryHandle, UsedLibraryAddressesIterator};
pub use lib_mappings::LibMappings;
pub use library_info::{LibraryInfo, Symbol, SymbolTable};
pub use markers::*;
Expand Down
8 changes: 6 additions & 2 deletions fxprof-processed-profile/src/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::cpu_delta::CpuDelta;
use crate::fast_hash_map::FastHashMap;
use crate::frame::{Frame, FrameInfo};
use crate::frame_table::{InternalFrame, InternalFrameLocation};
use crate::global_lib_table::{GlobalLibTable, LibraryHandle};
use crate::global_lib_table::{GlobalLibTable, LibraryHandle, UsedLibraryAddressesIterator};
use crate::lib_mappings::LibMappings;
use crate::library_info::LibraryInfo;
use crate::process::{Process, ThreadHandle};
Expand Down Expand Up @@ -601,7 +601,7 @@ impl Profile {
flags: frame_info.flags,
category_pair: frame_info.category_pair,
};
let frame_index = thread.frame_index_for_frame(internal_frame, &self.global_libs);
let frame_index = thread.frame_index_for_frame(internal_frame, &mut self.global_libs);
prefix =
Some(thread.stack_index_for_stack(prefix, frame_index, frame_info.category_pair));
}
Expand Down Expand Up @@ -665,6 +665,10 @@ impl Profile {
fn contains_js_function(&self) -> bool {
self.threads.iter().any(|t| t.contains_js_function())
}

pub fn lib_used_rva_iter(&self) -> UsedLibraryAddressesIterator {
self.global_libs.lib_used_rva_iter()
}
}

impl Serialize for Profile {
Expand Down
2 changes: 1 addition & 1 deletion fxprof-processed-profile/src/thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl Thread {
pub fn frame_index_for_frame(
&mut self,
frame: InternalFrame,
global_libs: &GlobalLibTable,
global_libs: &mut GlobalLibTable,
) -> usize {
self.frame_table.index_for_frame(
&mut self.string_table,
Expand Down
10 changes: 9 additions & 1 deletion samply-symbols/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ pub use crate::shared::{
MultiArchDisambiguator, OptionallySendFuture, PeCodeId, SourceFilePath, SymbolInfo,
SyncAddressInfo,
};
pub use crate::symbol_map::SymbolMap;
pub use crate::symbol_map::{SymbolMap, SymbolMapTrait};

pub struct SymbolManager<H: FileAndPathHelper> {
helper: Arc<H>,
Expand Down Expand Up @@ -304,6 +304,14 @@ where
/// Obtain a symbol map for the library, given the (partial) `LibraryInfo`.
/// At least the debug_id has to be given.
pub async fn load_symbol_map(&self, library_info: &LibraryInfo) -> Result<SymbolMap<H>, Error> {
if let Some((fl, symbol_map)) = self
.helper()
.as_ref()
.get_symbol_map_for_library(library_info)
{
return Ok(SymbolMap::with_symbol_map_trait(fl, symbol_map));
}

let debug_id = match library_info.debug_id {
Some(debug_id) => debug_id,
None => return Err(Error::NotEnoughInformationToIdentifySymbolMap),
Expand Down
10 changes: 10 additions & 0 deletions samply-symbols/src/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::future::Future;
use std::marker::PhantomData;
use std::ops::{Deref, Range};
use std::str::FromStr;
use std::sync::Arc;

#[cfg(feature = "partial_read_stats")]
use bitvec::{bitvec, prelude::BitVec};
Expand All @@ -14,6 +15,7 @@ use object::FileFlags;
use uuid::Uuid;

use crate::mapped_path::MappedPath;
use crate::symbol_map::SymbolMapTrait;

pub type FileAndPathHelperError = Box<dyn std::error::Error + Send + Sync + 'static>;
pub type FileAndPathHelperResult<T> = std::result::Result<T, FileAndPathHelperError>;
Expand Down Expand Up @@ -402,6 +404,14 @@ pub trait FileAndPathHelper {
&self,
location: Self::FL,
) -> std::pin::Pin<Box<dyn OptionallySendFuture<Output = FileAndPathHelperResult<Self::F>> + '_>>;

/// Ask the helper to return a SymbolMap if it happens to have one available already.
fn get_symbol_map_for_library(
&self,
_info: &LibraryInfo,
) -> Option<(Self::FL, Arc<dyn SymbolMapTrait + Send + Sync>)> {
None
}
}

/// Provides synchronous access to the raw bytes of a file.
Expand Down
17 changes: 15 additions & 2 deletions samply-symbols/src/symbol_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ pub trait GetInnerSymbolMapWithLookupFramesExt<FC> {
enum InnerSymbolMap<FC> {
WithoutAddFile(Box<dyn GetInnerSymbolMap + Send + Sync>),
WithAddFile(Box<dyn GetInnerSymbolMapWithLookupFramesExt<FC> + Send + Sync>),
Direct(Arc<dyn SymbolMapTrait + Send + Sync>),
}

pub struct SymbolMap<H: FileAndPathHelper> {
Expand Down Expand Up @@ -74,10 +75,22 @@ impl<H: FileAndPathHelper> SymbolMap<H> {
}
}

pub fn with_symbol_map_trait(
debug_file_location: H::FL,
inner: Arc<dyn SymbolMapTrait + Send + Sync>,
) -> Self {
Self {
debug_file_location,
inner: InnerSymbolMap::Direct(inner),
helper: None,
}
}

fn inner(&self) -> &dyn SymbolMapTrait {
match &self.inner {
InnerSymbolMap::WithoutAddFile(inner) => inner.get_inner_symbol_map(),
InnerSymbolMap::WithAddFile(inner) => inner.get_inner_symbol_map().get_as_symbol_map(),
InnerSymbolMap::Direct(inner) => inner.as_ref(),
}
}

Expand Down Expand Up @@ -111,7 +124,7 @@ impl<H: FileAndPathHelper> SymbolMap<H> {
frames: Some(frames),
});
}
(None, _) | (_, InnerSymbolMap::WithoutAddFile(_)) => {
(None, _) | (_, InnerSymbolMap::WithoutAddFile(_)) | (_, InnerSymbolMap::Direct(_)) => {
return Some(AddressInfo {
symbol,
frames: None,
Expand Down Expand Up @@ -168,7 +181,7 @@ impl<H: FileAndPathHelper> SymbolMap<H> {
) -> Option<Vec<FrameDebugInfo>> {
let helper = self.helper.as_deref()?;
let inner = match &self.inner {
InnerSymbolMap::WithoutAddFile(_) => return None,
InnerSymbolMap::WithoutAddFile(_) | InnerSymbolMap::Direct(_) => return None,
InnerSymbolMap::WithAddFile(inner) => inner.get_inner_symbol_map(),
};

Expand Down
21 changes: 18 additions & 3 deletions samply/src/linux/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ pub fn start_recording(
let interval = recording_props.interval;
let time_limit = recording_props.time_limit;
let observer_thread = thread::spawn(move || {
let unstable_presymbolicate = profile_creation_props.unstable_presymbolicate;
let mut converter = make_converter(interval, profile_creation_props);

// Wait for the initial pid to profile.
Expand Down Expand Up @@ -120,6 +121,7 @@ pub fn start_recording(
profile_another_pid_request_receiver,
profile_another_pid_reply_sender,
stop_receiver,
unstable_presymbolicate,
);
});

Expand Down Expand Up @@ -257,6 +259,7 @@ fn start_profiling_pid(
move || {
let interval = recording_props.interval;
let time_limit = recording_props.time_limit;
let unstable_presymbolicate = profile_creation_props.unstable_presymbolicate;
let mut converter = make_converter(interval, profile_creation_props);
let SamplerRequest::StartProfilingAnotherProcess(pid, attach_mode) =
profile_another_pid_request_receiver.recv().unwrap()
Expand All @@ -277,6 +280,7 @@ fn start_profiling_pid(
profile_another_pid_request_receiver,
profile_another_pid_reply_sender,
ctrl_c_receiver,
unstable_presymbolicate,
)
}
});
Expand Down Expand Up @@ -524,6 +528,7 @@ enum SamplerRequest {
StopProfilingOncePerfEventsExhausted,
}

#[allow(clippy::too_many_arguments)]
fn run_profiler(
mut perf: PerfGroup,
mut converter: Converter<
Expand All @@ -534,6 +539,7 @@ fn run_profiler(
more_processes_request_receiver: Receiver<SamplerRequest>,
more_processes_reply_sender: Sender<bool>,
mut stop_receiver: oneshot::Receiver<()>,
unstable_presymbolicate: bool,
) {
// eprintln!("Running...");

Expand Down Expand Up @@ -659,9 +665,18 @@ fn run_profiler(

let profile = converter.finish();

let output_file = File::create(output_filename).unwrap();
let writer = BufWriter::new(output_file);
serde_json::to_writer(writer, &profile).expect("Couldn't write JSON");
{
let output_file = File::create(output_filename).unwrap();
let writer = BufWriter::new(output_file);
serde_json::to_writer(writer, &profile).expect("Couldn't write JSON");
}

if unstable_presymbolicate {
crate::shared::symbol_precog::presymbolicate(
&profile,
&output_filename.with_extension("syms.json"),
);
}
}

pub fn read_string_lossy<P: AsRef<Path>>(path: P) -> std::io::Result<String> {
Expand Down
9 changes: 9 additions & 0 deletions samply/src/mac/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ pub fn start_recording(
..profile_creation_props
};

let unstable_presymbolicate = profile_creation_props.unstable_presymbolicate;

let (task_sender, task_receiver) = unbounded();

let sampler_thread = thread::spawn(move || {
Expand Down Expand Up @@ -210,6 +212,13 @@ pub fn start_recording(
to_writer(writer, &profile).expect("Couldn't write JSON");
}

if unstable_presymbolicate {
crate::shared::symbol_precog::presymbolicate(
&profile,
&output_file.with_extension("syms.json"),
);
}

if let Some(server_props) = server_props {
let libinfo_map = crate::profile_json_preparse::parse_libinfo_map_from_profile_file(
File::open(&output_file).expect("Couldn't open file we just wrote"),
Expand Down
10 changes: 10 additions & 0 deletions samply/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,14 @@ pub struct ProfileCreationArgs {
/// Create a separate thread for each CPU. Not supported on macOS
#[arg(long)]
per_cpu_threads: bool,

/// Emit .syms.json sidecar file containing gathered symbol info for all frames referenced by
/// this profile. With this file along with the profile, samply can load the profile
/// and provide symbols to the front end without needing debug files to be
/// available. (Unstable: will probably change to include the full information
/// in the profile.json, instead of a sidecar file.)
#[arg(long)]
unstable_presymbolicate: bool,
}

#[derive(Debug, Args)]
Expand Down Expand Up @@ -369,6 +377,7 @@ impl ImportArgs {
unlink_aux_files: self.profile_creation_args.unlink_aux_files,
create_per_cpu_threads: self.profile_creation_args.per_cpu_threads,
override_arch: self.override_arch.clone(),
unstable_presymbolicate: self.profile_creation_args.unstable_presymbolicate,
}
}

Expand Down Expand Up @@ -472,6 +481,7 @@ impl RecordArgs {
unlink_aux_files: self.profile_creation_args.unlink_aux_files,
create_per_cpu_threads: self.profile_creation_args.per_cpu_threads,
override_arch: None,
unstable_presymbolicate: self.profile_creation_args.unstable_presymbolicate,
}
}
}
Expand Down
Loading