diff --git a/fxprof-processed-profile/src/global_lib_table.rs b/fxprof-processed-profile/src/global_lib_table.rs index e5d96d30..cc38c467 100644 --- a/fxprof-processed-profile/src/global_lib_table.rs +++ b/fxprof-processed-profile/src/global_lib_table.rs @@ -3,17 +3,18 @@ use std::sync::Arc; use serde::ser::{Serialize, Serializer}; -use crate::fast_hash_map::FastHashMap; +use crate::fast_hash_map::{FastHashMap, FastIndexSet}; use crate::{LibraryInfo, SymbolTable}; #[derive(Debug)] pub struct GlobalLibTable { - /// All libraries added via `Profile::add_lib`. May or may not be used. + /// All libraries added via `Profile::handle_for_lib`. May or may not be used. /// Indexed by `LibraryHandle.0`. - all_libs: Vec, // append-only for stable LibraryHandles + all_libs: FastIndexSet, // append-only for stable LibraryHandles + /// Any symbol tables for libraries in all_libs + symbol_tables: FastHashMap>, /// Indexed by `GlobalLibIndex.0`. used_libs: Vec, // append-only for stable GlobalLibIndexes - lib_map: FastHashMap, used_lib_map: FastHashMap, /// 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. @@ -26,25 +27,20 @@ pub struct GlobalLibTable { impl GlobalLibTable { pub fn new() -> Self { Self { - all_libs: Vec::new(), + all_libs: FastIndexSet::default(), + symbol_tables: FastHashMap::default(), used_libs: Vec::new(), - lib_map: FastHashMap::default(), used_lib_map: FastHashMap::default(), used_libs_seen_rvas: Vec::new(), } } pub fn handle_for_lib(&mut self, lib: LibraryInfo) -> LibraryHandle { - let all_libs = &mut self.all_libs; - *self.lib_map.entry(lib.clone()).or_insert_with(|| { - let handle = LibraryHandle(all_libs.len()); - all_libs.push(lib); - handle - }) + LibraryHandle(self.all_libs.insert_full(lib).0) } pub fn set_lib_symbol_table(&mut self, library: LibraryHandle, symbol_table: Arc) { - self.all_libs[library.0].symbol_table = Some(symbol_table); + self.symbol_tables.insert(library, symbol_table); } pub fn index_for_used_lib(&mut self, lib_handle: LibraryHandle) -> GlobalLibIndex { @@ -59,7 +55,12 @@ impl GlobalLibTable { pub fn get_lib(&self, index: GlobalLibIndex) -> Option<&LibraryInfo> { let handle = self.used_libs.get(index.0)?; - self.all_libs.get(handle.0) + self.all_libs.get_index(handle.0) + } + + pub fn get_lib_symbol_table(&self, index: GlobalLibIndex) -> Option<&SymbolTable> { + let handle = self.used_libs.get(index.0)?; + self.symbol_tables.get(handle).map(|v| &**v) } pub fn add_lib_used_rva(&mut self, index: GlobalLibIndex, address: u32) { diff --git a/fxprof-processed-profile/src/library_info.rs b/fxprof-processed-profile/src/library_info.rs index 801dfcd9..68fc8b1e 100644 --- a/fxprof-processed-profile/src/library_info.rs +++ b/fxprof-processed-profile/src/library_info.rs @@ -1,5 +1,3 @@ -use std::sync::Arc; - use debugid::DebugId; use serde::ser::{Serialize, SerializeMap, Serializer}; @@ -40,24 +38,6 @@ pub struct LibraryInfo { /// correct sub-binary in a mach-O fat binary. But we now use the debug_id for that /// purpose. pub arch: Option, - /// An optional symbol table, for "pre-symbolicating" stack frames. - /// - /// Usually, symbolication is something that should happen asynchronously, - /// because it can be very slow, so the regular way to use the profiler is to - /// store only frame addresses and no symbols in the profile JSON, and perform - /// symbolication only once the profile is loaded in the Firefox Profiler UI. - /// - /// However, sometimes symbols are only available during recording and are not - /// easily accessible afterwards. One such example the symbol table of the - /// Linux kernel: Users with root privileges can access the symbol table of the - /// currently-running kernel via `/proc/kallsyms`, but we don't want to have - /// to run the local symbol server with root privileges. So it's easier to - /// resolve kernel symbols when generating the profile JSON. - /// - /// This way of symbolicating does not support file names, line numbers, or - /// inline frames. It is intended for relatively "small" symbol tables for which - /// an address lookup is fast. - pub symbol_table: Option>, } impl Serialize for LibraryInfo { diff --git a/fxprof-processed-profile/src/profile.rs b/fxprof-processed-profile/src/profile.rs index c1aa76d0..dfc69589 100644 --- a/fxprof-processed-profile/src/profile.rs +++ b/fxprof-processed-profile/src/profile.rs @@ -382,15 +382,23 @@ impl Profile { self.global_libs.handle_for_lib(library) } - /// Set the symbol table for a library. - /// - /// This symbol table can also be specified in the [`LibraryInfo`] which is given to - /// [`Profile::add_lib`]. However, sometimes you may want to have the [`LibraryHandle`] - /// for a library before you know about all its symbols. In those cases, you can call - /// [`Profile::add_lib`] with `symbol_table` set to `None`, and then supply the symbol - /// table afterwards. - /// - /// Symbol tables are optional. + /// Set an optional symbol table for a library, for "pre-symbolicating" stack frames. + /// + /// Usually, symbolication is something that should happen asynchronously, + /// because it can be very slow, so the regular way to use the profiler is to + /// store only frame addresses and no symbols in the profile JSON, and perform + /// symbolication only once the profile is loaded in the Firefox Profiler UI. + /// + /// However, sometimes symbols are only available during recording and are not + /// easily accessible afterwards. One such example the symbol table of the + /// Linux kernel: Users with root privileges can access the symbol table of the + /// currently-running kernel via `/proc/kallsyms`, but we don't want to have + /// to run the local symbol server with root privileges. So it's easier to + /// resolve kernel symbols when generating the profile JSON. + /// + /// This form of symbolicating does not support file names, line numbers, or + /// inline frames. It is intended for relatively "small" symbol tables for which + /// an address lookup is fast. pub fn set_lib_symbol_table(&mut self, library: LibraryHandle, symbol_table: Arc) { self.global_libs.set_lib_symbol_table(library, symbol_table); } @@ -598,11 +606,8 @@ impl Profile { (InternalFrameVariant::Label, name) } InternalFrameAddress::InLib(address, lib_index) => { - let lib = self.global_libs.get_lib(lib_index).unwrap(); - let symbol = lib - .symbol_table - .as_deref() - .and_then(|symbol_table| symbol_table.lookup(address)); + let lib_symbol_table = self.global_libs.get_lib_symbol_table(lib_index); + let symbol = lib_symbol_table.and_then(|symbol_table| symbol_table.lookup(address)); let (native_symbol, name) = match symbol { Some(symbol) => { let (native_symbol, name) = thread diff --git a/fxprof-processed-profile/tests/integration_tests/main.rs b/fxprof-processed-profile/tests/integration_tests/main.rs index 55a94d25..2c239e2c 100644 --- a/fxprof-processed-profile/tests/integration_tests/main.rs +++ b/fxprof-processed-profile/tests/integration_tests/main.rs @@ -142,7 +142,10 @@ fn profile_without_js() { debug_path: "/usr/lib/x86_64-linux-gnu/libc.so.6".to_string(), debug_id: DebugId::from_breakpad("1629FCF0BE5C8860C0E1ADF03B0048FB0").unwrap(), arch: None, - symbol_table: Some(Arc::new(SymbolTable::new(vec![ + }); + profile.set_lib_symbol_table( + libc_handle, + Arc::new(SymbolTable::new(vec![ Symbol { address: 1700001, size: Some(180), @@ -158,8 +161,8 @@ fn profile_without_js() { size: Some(20), name: "libc_symbol_2".to_string(), }, - ]))), - }); + ])), + ); profile.add_lib_mapping( process, libc_handle, @@ -175,7 +178,6 @@ fn profile_without_js() { debug_path: "/home/mstange/code/dump_syms/target/release/dump_syms".to_string(), debug_id: DebugId::from_breakpad("5C0A0D51EA1980DF43F203B4525BE9BE0").unwrap(), arch: None, - symbol_table: None, }); profile.add_lib_mapping( process, diff --git a/samply/src/linux_shared/converter.rs b/samply/src/linux_shared/converter.rs index d8b52307..62526002 100644 --- a/samply/src/linux_shared/converter.rs +++ b/samply/src/linux_shared/converter.rs @@ -1220,8 +1220,10 @@ where name: dso_key.name().to_string(), debug_name: dso_key.name().to_string(), arch: None, - symbol_table, }); + if let Some(symbol_table) = symbol_table { + self.profile.set_lib_symbol_table(lib_handle, symbol_table); + } let end_address = base_address + len; self.profile .add_kernel_lib_mapping(lib_handle, base_address, end_address, 0); @@ -1390,8 +1392,9 @@ where debug_name: name.clone(), name, arch: None, - symbol_table: Some(symbol_table.symbol_table.clone()), }); + self.profile + .set_lib_symbol_table(lib_handle, symbol_table.symbol_table.clone()); let info = match symbol_table.art_info { Some(AndroidArtInfo::LibArt) => LibMappingInfo::new_libart_mapping(lib_handle), Some(AndroidArtInfo::JavaFrame) => { @@ -1556,7 +1559,6 @@ where debug_name: name.clone(), name, arch: None, - symbol_table: None, }); process.add_regular_lib_mapping( timestamp, @@ -1607,7 +1609,6 @@ where debug_name: name.to_owned(), name: name.to_owned(), arch: None, - symbol_table: None, }) } diff --git a/samply/src/mac/task_profiler.rs b/samply/src/mac/task_profiler.rs index b78a85aa..18623c30 100644 --- a/samply/src/mac/task_profiler.rs +++ b/samply/src/mac/task_profiler.rs @@ -462,7 +462,6 @@ impl TaskProfiler { debug_id: lib.debug_id.unwrap(), code_id: lib.code_id.map(|ci| ci.to_string()), arch: lib.arch.map(ToOwned::to_owned), - symbol_table: None, }); self.lib_mapping_ops.push( now_mono, diff --git a/samply/src/shared/perf_map.rs b/samply/src/shared/perf_map.rs index a4199d63..5c53b480 100644 --- a/samply/src/shared/perf_map.rs +++ b/samply/src/shared/perf_map.rs @@ -62,7 +62,6 @@ pub fn try_load_perf_map( debug_id: DebugId::nil(), code_id: None, arch: None, - symbol_table: None, }); let mut symbols = Vec::new(); diff --git a/samply/src/shared/synthetic_jit_library.rs b/samply/src/shared/synthetic_jit_library.rs index e54d0949..03a814e0 100644 --- a/samply/src/shared/synthetic_jit_library.rs +++ b/samply/src/shared/synthetic_jit_library.rs @@ -31,7 +31,6 @@ impl SyntheticJitLibrary { debug_id: DebugId::nil(), code_id: None, arch: None, - symbol_table: None, }); let recycler = if allow_recycling { Some(FastHashMap::default()) diff --git a/samply/src/shared/utils.rs b/samply/src/shared/utils.rs index e1329c13..6c3e1581 100644 --- a/samply/src/shared/utils.rs +++ b/samply/src/shared/utils.rs @@ -49,6 +49,5 @@ pub fn lib_handle_for_jitdump( debug_id, code_id: Some(code_id.to_string()), arch: None, - symbol_table: None, }) } diff --git a/samply/src/windows/profile_context.rs b/samply/src/windows/profile_context.rs index 2f1d0ff5..4154ef87 100644 --- a/samply/src/windows/profile_context.rs +++ b/samply/src/windows/profile_context.rs @@ -1476,7 +1476,6 @@ impl ProfileContext { debug_id, code_id: code_id.map(|ci| ci.to_string()), arch: Some(self.arch.to_owned()), - symbol_table: None, }); // attempt to categorize the library based on the path