Store symbol tables separately from fxprof_processed_profile::LibraryInfo.

mstange · mstange · commit 296b78adcd3a · 2025-04-15T08:38:17.000-04:00
We already have a separate Profile::set_lib_symbol_table method so just use that always.
Most consumers just specify None, and now they don't have to do that anymore.

But the main motivation for this change was performance: We were hashing the symbol
table during calls to handle_for_lib. This fixes that.
diff --git 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<LibraryInfo>, // append-only for stable LibraryHandles
+    all_libs: FastIndexSet<LibraryInfo>, // append-only for stable LibraryHandles
+    /// Any symbol tables for libraries in all_libs
+    symbol_tables: FastHashMap<LibraryHandle, Arc<SymbolTable>>,
     /// Indexed by `GlobalLibIndex.0`.
     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.
@@ -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<SymbolTable>) {
-        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
@@ -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<String>,
-    /// 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<Arc<SymbolTable>>,
 }
 
 impl Serialize for LibraryInfo {
diff --git 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<SymbolTable>) {
         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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -49,6 +49,5 @@ pub fn lib_handle_for_jitdump(`
`49`	`49`	`debug_id,`
`50`	`50`	`code_id: Some(code_id.to_string()),`
`51`	`51`	`arch: None,`
`52`		`- symbol_table: None,`
`53`	`52`	`})`
`54`	`53`	`}`