Some more breakpad sym file symbolication improvements (#678)

This should reduce memory usage when symbols come from breakpad sym
files.

Author: mstange

samply-api/src/symbolicate/looked_up_addresses.rs

@@ -1,6 +1,6 @@
 use std::collections::BTreeMap;
 
-use samply_symbols::{FrameDebugInfo, SourceFilePath, SourceFilePathHandle};
+use samply_symbols::{FrameDebugInfo, SourceFilePath, SourceFilePathHandle, SymbolInfo};
 
 pub trait PathResolver {
     fn resolve_source_file_path(&self, handle: SourceFilePathHandle) -> SourceFilePath<'_>;
@@ -13,71 +13,27 @@ impl PathResolver for () {
 }
 
 pub struct AddressResult {
-    pub symbol_address: u32,
-    pub symbol_name: String,
-    pub function_size: Option<u32>,
+    pub symbol: SymbolInfo,
     pub inline_frames: Option<Vec<FrameDebugInfo>>,
 }
 
-pub type AddressResults = BTreeMap<u32, Option<AddressResult>>;
-
-pub struct LookedUpAddresses {
-    pub address_results: AddressResults,
-    pub symbol_count: u32,
-}
-
-impl LookedUpAddresses {
-    pub fn for_addresses(addresses: &[u32]) -> Self {
-        LookedUpAddresses {
-            address_results: addresses.iter().map(|&addr| (addr, None)).collect(),
-            symbol_count: 0,
-        }
-    }
-
-    pub fn add_address_symbol(
-        &mut self,
-        address: u32,
-        symbol_address: u32,
-        symbol_name: String,
-        function_size: Option<u32>,
-    ) {
-        *self.address_results.get_mut(&address).unwrap() = Some(AddressResult {
-            symbol_address,
-            symbol_name,
-            function_size,
+impl AddressResult {
+    pub fn new(symbol: SymbolInfo) -> Self {
+        Self {
+            symbol,
             inline_frames: None,
-        });
-    }
-
-    pub fn add_address_debug_info(&mut self, address: u32, frames: Vec<FrameDebugInfo>) {
-        let outer_function_name = frames.last().and_then(|f| f.function.as_deref());
-        let entry = self.address_results.get_mut(&address).unwrap();
-
-        match entry {
-            Some(address_result) => {
-                // Overwrite the symbol name with the function name from the debug info.
-                if let Some(name) = outer_function_name {
-                    address_result.symbol_name = name.to_string();
-                }
-                // Add the inline frame info.``
-                address_result.inline_frames = Some(frames);
-            }
-            None => {
-                // add_address_symbol has not been called for this address.
-                // This happens when we only have debug info but no symbol for this address.
-                // This is a rare case.
-                *entry = Some(AddressResult {
-                    symbol_address: address, // TODO: Would be nice to get the actual function start address from addr2line
-                    symbol_name: outer_function_name
-                        .map_or_else(|| format!("0x{address:x}"), str::to_string),
-                    function_size: None,
-                    inline_frames: Some(frames),
-                });
-            }
         }
     }
 
-    pub fn set_total_symbol_count(&mut self, total_symbol_count: u32) {
-        self.symbol_count = total_symbol_count;
+    pub fn set_debug_info(&mut self, frames: Vec<FrameDebugInfo>) {
+        let outer_function_name = frames.last().and_then(|f| f.function.as_ref());
+        // Overwrite the symbol name with the function name from the debug info.
+        if let Some(name) = outer_function_name {
+            self.symbol.name = name.clone();
+        }
+        // Add the inline frame info.
+        self.inline_frames = Some(frames);
     }
 }
+
+pub type AddressResults = BTreeMap<u32, Option<AddressResult>>;

samply-api/src/symbolicate/mod.rs

@@ -2,20 +2,19 @@ use std::collections::HashMap;
 use std::sync::Arc;
 
 use samply_symbols::{
-    FileAndPathHelper, FramesLookupResult, LibraryInfo, LookupAddress, SourceFilePath,
-    SourceFilePathHandle, SymbolManager, SymbolMap,
+    AccessPatternHint, FileAndPathHelper, FramesLookupResult, LibraryInfo, LookupAddress,
+    SourceFilePath, SourceFilePathHandle, SymbolManager, SymbolMap,
 };
 
 use crate::error::Error;
-use crate::symbolicate::looked_up_addresses::PathResolver;
+use crate::symbolicate::looked_up_addresses::{AddressResult, AddressResults, PathResolver};
 use crate::symbolicate::response_json::{PerLibResult, Response};
 use crate::to_debug_id;
 
 pub mod looked_up_addresses;
 pub mod request_json;
 pub mod response_json;
 
-use looked_up_addresses::LookedUpAddresses;
 use request_json::Lib;
 
 impl<H: FileAndPathHelper> PathResolver for SymbolMap<H> {
@@ -63,7 +62,7 @@ impl<'a, H: FileAndPathHelper + 'static> SymbolicateApi<'a, H> {
         let mut symbolicated_addresses = HashMap::new();
         for (lib, addresses) in requested_addresses.into_iter() {
             let address_results = self
-                .symbolicate_requested_addresses_for_lib(&lib, addresses)
+                .symbolicate_requested_addresses_for_lib(&lib, &addresses)
                 .await;
             symbolicated_addresses.insert(lib, address_results);
         }
@@ -73,48 +72,42 @@ impl<'a, H: FileAndPathHelper + 'static> SymbolicateApi<'a, H> {
     async fn symbolicate_requested_addresses_for_lib(
         &self,
         lib: &Lib,
-        mut addresses: Vec<u32>,
+        addresses: &[u32],
     ) -> Result<PerLibResult<H>, samply_symbols::Error> {
-        // Sort the addresses before the lookup, to have a higher chance of hitting
-        // the same external file for subsequent addresses.
-        addresses.sort_unstable();
-        addresses.dedup();
-
         let debug_id = to_debug_id(&lib.breakpad_id)?;
 
-        let mut external_addresses = Vec::new();
-
-        // Do the synchronous work first, and accumulate external_addresses which need
-        // to be handled asynchronously. This allows us to group async file loads by
-        // the external file.
-
         let info = LibraryInfo {
             debug_name: Some(lib.debug_name.to_string()),
             debug_id: Some(debug_id),
             ..Default::default()
         };
         let symbol_map = self.symbol_manager.load_symbol_map(&info).await?;
-        let mut symbolication_result = LookedUpAddresses::for_addresses(&addresses);
-
-        symbolication_result.set_total_symbol_count(symbol_map.symbol_count() as u32);
-
-        for &address in &addresses {
-            if let Some(address_info) = symbol_map.lookup_sync(LookupAddress::Relative(address)) {
-                symbolication_result.add_address_symbol(
-                    address,
-                    address_info.symbol.address,
-                    address_info.symbol.name,
-                    address_info.symbol.size,
-                );
-                match address_info.frames {
-                    Some(FramesLookupResult::Available(frames)) => {
-                        symbolication_result.add_address_debug_info(address, frames)
-                    }
-                    Some(FramesLookupResult::External(ext_address)) => {
-                        external_addresses.push((address, ext_address));
-                    }
-                    None => {}
+
+        // Create a BTreeMap for the lookup results. This lets us iterate over the
+        // addresses in ascending order - the sort happens when the map is created.
+        let mut address_results: AddressResults =
+            addresses.iter().map(|&addr| (addr, None)).collect();
+        symbol_map.set_access_pattern_hint(AccessPatternHint::SequentialLookup);
+
+        // Do the synchronous work first, and accumulate external_addresses which need
+        // to be handled asynchronously. This allows us to group async file loads by
+        // the external file.
+        let mut external_addresses = Vec::new();
+
+        for (&address, address_result) in &mut address_results {
+            let Some(address_info) = symbol_map.lookup_sync(LookupAddress::Relative(address))
+            else {
+                continue;
+            };
+            *address_result = Some(AddressResult::new(address_info.symbol));
+            match address_info.frames {
+                Some(FramesLookupResult::Available(frames)) => {
+                    address_result.as_mut().unwrap().set_debug_info(frames)
+                }
+                Some(FramesLookupResult::External(ext_address)) => {
+                    external_addresses.push((address, ext_address));
                 }
+                None => {}
             }
         }
 
@@ -126,12 +119,13 @@ impl<'a, H: FileAndPathHelper + 'static> SymbolicateApi<'a, H> {
 
         for (address, ext_address) in external_addresses {
             if let Some(frames) = symbol_map.lookup_external(&ext_address).await {
-                symbolication_result.add_address_debug_info(address, frames);
+                let address_result = address_results.get_mut(&address).unwrap();
+                address_result.as_mut().unwrap().set_debug_info(frames);
             }
         }
 
         let outcome = PerLibResult {
-            address_results: symbolication_result,
+            address_results,
             symbol_map: Arc::new(symbol_map),
         };
 

samply-api/src/symbolicate/response_json.rs

@@ -5,14 +5,13 @@ use std::sync::Arc;
 use samply_symbols::{FileAndPathHelper, FrameDebugInfo, SymbolMap};
 use serde::ser::{SerializeMap, SerializeSeq};
 
-use super::looked_up_addresses::LookedUpAddresses;
 use super::request_json::{Job, Lib, RequestFrame, RequestStack};
 use crate::api_file_path::to_api_file_path;
 use crate::symbolicate::looked_up_addresses::{AddressResults, PathResolver};
 use crate::symbolicate::request_json::Request;
 
 pub struct PerLibResult<H: FileAndPathHelper> {
-    pub address_results: LookedUpAddresses,
+    pub address_results: AddressResults,
     pub symbol_map: Arc<SymbolMap<H>>,
 }
 
@@ -59,7 +58,7 @@ impl<H: FileAndPathHelper> serde::Serialize for Response<H> {
 }
 
 pub struct PerLibResultRef<'a> {
-    pub address_results: std::result::Result<&'a LookedUpAddresses, &'a samply_symbols::Error>,
+    pub address_results: std::result::Result<&'a AddressResults, &'a samply_symbols::Error>,
     pub path_resolver: &'a dyn PathResolver,
 }
 
@@ -106,9 +105,9 @@ impl<'a> serde::Serialize for Result<'a> {
             if let Some(per_lib_result) = self.per_lib_results.get(lib) {
                 let module_key = format!("{}/{}", lib.debug_name, lib.breakpad_id);
                 match per_lib_result.address_results {
-                    Ok(symbols) => {
+                    Ok(address_results) => {
                         let module_results = PerModuleResultsRef {
-                            address_results: &symbols.address_results,
+                            address_results,
                             path_resolver: per_lib_result.path_resolver,
                         };
                         symbols_by_module_index.insert(module_index as u32, module_results);
@@ -213,12 +212,12 @@ impl<'a> serde::Serialize for ResponseFrame<'a> {
             let address_result = symbol_map.address_results.get(&frame.address).unwrap();
             // But the result might still be None.
             if let Some(address_result) = address_result {
-                map.serialize_entry("function", &address_result.symbol_name)?;
+                map.serialize_entry("function", &address_result.symbol.name)?;
                 map.serialize_entry(
                     "function_offset",
-                    &SerializeAsHexStr(frame.address - address_result.symbol_address),
+                    &SerializeAsHexStr(frame.address - address_result.symbol.address),
                 )?;
-                if let Some(function_size) = address_result.function_size {
+                if let Some(function_size) = address_result.symbol.size {
                     map.serialize_entry("function_size", &SerializeAsHexStr(function_size))?;
                 }