feat: Use proguard cache format (#1491)

Based on getsentry/rust-proguard#42.

This should significantly increase proguard processing performance by eliminating the time it takes to parse proguard files, at a slight cost to remapping itself.

crates/symbolicator-proguard/Cargo.toml

@@ -8,7 +8,7 @@ license = "MIT"
 
 [dependencies]
 futures = "0.3.12"
-proguard = "5.4.1"
+proguard = "5.5.0"
 serde = { version = "1.0.137", features = ["derive", "rc"] }
 serde_json = "1.0.81"
 symbolic = "12.8.0"

crates/symbolicator-proguard/src/service.rs

@@ -1,12 +1,14 @@
+use std::io::BufWriter;
 use std::sync::Arc;
 
 use futures::future::BoxFuture;
+use proguard::ProguardCache;
 use symbolic::common::{AsSelf, ByteView, DebugId, SelfCell};
 use symbolicator_service::caches::versions::PROGUARD_CACHE_VERSIONS;
 use symbolicator_service::caching::{
  CacheEntry, CacheError, CacheItemRequest, CacheKey, CacheVersions, Cacher,
 };
-use symbolicator_service::download::{fetch_file, DownloadService};
+use symbolicator_service::download::{fetch_file, tempfile_in_parent, DownloadService};
 use symbolicator_service::objects::{
  FindObject, FindResult, ObjectHandle, ObjectPurpose, ObjectsActor,
 };
@@ -46,7 +48,7 @@ impl ProguardService {
  sources: &[SourceConfig],
  scope: &Scope,
  debug_id: DebugId,
- ) -> CacheEntry<ProguardMapper> {
+ ) -> CacheEntry<OwnedProguardCache> {
  let identifier = ObjectId {
  debug_id: Some(debug_id),
  ..Default::default()
@@ -67,10 +69,7 @@ impl ProguardService {
  download_svc: Arc::clone(&self.download_svc),
  };
 
- self.cache
- .compute_memoized(request, cache_key)
- .await
- .map(|item| item.1)
+ self.cache.compute_memoized(request, cache_key).await
  }
 
  /// Downloads a source bundle for the given scope and debug id.
@@ -105,7 +104,7 @@ impl ProguardService {
 }
 
 struct ProguardInner<'a> {
- mapper: proguard::ProguardMapper<'a>,
+ cache: proguard::ProguardCache<'a>,
 }
 
 impl<'slf, 'a: 'slf> AsSelf<'slf> for ProguardInner<'a> {
@@ -117,26 +116,28 @@ impl<'slf, 'a: 'slf> AsSelf<'slf> for ProguardInner<'a> {
 }
 
 #[derive(Clone)]
-pub struct ProguardMapper {
+pub struct OwnedProguardCache {
  inner: Arc<SelfCell<ByteView<'static>, ProguardInner<'static>>>,
 }
 
-impl ProguardMapper {
- #[tracing::instrument(name = "ProguardMapper::new", skip_all, fields(size = byteview.len()))]
- pub fn new(byteview: ByteView<'static>) -> Self {
- let inner = SelfCell::new(byteview, |data| {
- let mapping = proguard::ProguardMapping::new(unsafe { &*data });
- let mapper = proguard::ProguardMapper::new_with_param_mapping(mapping, true);
- ProguardInner { mapper }
- });
-
- Self {
+impl OwnedProguardCache {
+ #[tracing::instrument(name = "OwnedProguardCache::new", skip_all, fields(size = byteview.len()))]
+ pub fn new(byteview: ByteView<'static>) -> Result<Self, CacheError> {
+ let inner = SelfCell::try_new::<CacheError, _>(byteview, |data| {
+ let cache = ProguardCache::parse(unsafe { &*data }).map_err(|e| {
+ tracing::error!(error = %e);
+ CacheError::InternalError
+ })?;
+ Ok(ProguardInner { cache })
+ })?;
+
+ Ok(Self {
  inner: Arc::new(inner),
- }
+ })
  }
 
- pub fn get(&self) -> &proguard::ProguardMapper {
- &self.inner.get().mapper
+ pub fn get(&self) -> &proguard::ProguardCache {
+ &self.inner.get().cache
  }
 }
 
@@ -147,9 +148,7 @@ pub struct FetchProguard {
 }
 
 impl CacheItemRequest for FetchProguard {
- /// The first component is the estimated memory footprint of the mapper,
- /// computed as 2x the size of the mapping file on disk.
- type Item = (u32, ProguardMapper);
+ type Item = OwnedProguardCache;
 
  const VERSIONS: CacheVersions = PROGUARD_CACHE_VERSIONS;
 
@@ -169,25 +168,23 @@ impl CacheItemRequest for FetchProguard {
  "The ProGuard file doesn't contain any line mappings".into(),
  ))
  } else {
+ let cache_temp_file = tempfile_in_parent(temp_file)?;
+ let mut writer = BufWriter::new(cache_temp_file);
+ ProguardCache::write(&mapping, &mut writer)?;
+ let mut cache_temp_file =
+ writer.into_inner().map_err(|_| CacheError::InternalError)?;
+ std::mem::swap(temp_file, &mut cache_temp_file);
  Ok(())
  }
  };
  Box::pin(fut)
  }
 
  fn load(&self, byteview: ByteView<'static>) -> CacheEntry<Self::Item> {
- let weight = byteview.len().try_into().unwrap_or(u32::MAX);
- // NOTE: In an extremely unscientific test, the proguard mapper was slightly less
- // than twice as big in memory as the file on disk.
- let weight = weight.saturating_mul(2);
- Ok((weight, ProguardMapper::new(byteview)))
+ OwnedProguardCache::new(byteview)
  }
 
  fn use_shared_cache(&self) -> bool {
  false
  }
-
- fn weight(item: &Self::Item) -> u32 {
- item.0.max(std::mem::size_of::<Self::Item>() as u32)
- }
 }

crates/symbolicator-proguard/src/symbolication.rs

@@ -154,7 +154,7 @@ impl ProguardService {
  /// Returns `None` if none of the `mappers` can remap the exception.
  #[tracing::instrument(skip_all)]
  fn map_exception(
- mappers: &[&proguard::ProguardMapper],
+ mappers: &[&proguard::ProguardCache],
  exception: &JvmException,
  ) -> Option<JvmException> {
  if mappers.is_empty() {
@@ -190,7 +190,7 @@ impl ProguardService {
  /// frame is returned.
  #[tracing::instrument(skip_all)]
  fn map_frame(
- mappers: &[&proguard::ProguardMapper],
+ mappers: &[&proguard::ProguardCache],
  frame: &JvmFrame,
  release_package: Option<&str>,
  ) -> Vec<JvmFrame> {
@@ -289,7 +289,7 @@ impl ProguardService {
  /// by `remap_frame`.
  #[tracing::instrument(skip_all)]
  fn map_full_frame<'a>(
- mapper: &'a proguard::ProguardMapper<'a>,
+ mapper: &'a proguard::ProguardCache<'a>,
  original_frame: &JvmFrame,
  proguard_frame: &proguard::StackFrame<'a>,
  buf: &mut Vec<proguard::StackFrame<'a>>,
@@ -338,7 +338,7 @@ impl ProguardService {
  /// Tries to remap a frame's class and method.
  #[tracing::instrument(skip_all)]
  fn map_class_method(
- mapper: &proguard::ProguardMapper,
+ mapper: &proguard::ProguardCache,
  frame: &JvmFrame,
  ) -> Option<Vec<JvmFrame>> {
  let (mapped_class, mapped_method) = mapper.remap_method(&frame.module, &frame.function)?;
@@ -351,7 +351,7 @@ impl ProguardService {
  }
 
  /// Tries to remap a frame's class.
- fn map_class(mapper: &proguard::ProguardMapper, frame: &JvmFrame) -> Option<Vec<JvmFrame>> {
+ fn map_class(mapper: &proguard::ProguardCache, frame: &JvmFrame) -> Option<Vec<JvmFrame>> {
  let mapped_class = mapper.remap_class(&frame.module)?;
 
  Some(vec![JvmFrame {
@@ -434,7 +434,7 @@ fn build_source_file_name(frame: &JvmFrame) -> String {
 #[cfg(test)]
 mod tests {
  use super::*;
- use proguard::{ProguardMapper, ProguardMapping};
+ use proguard::{ProguardCache, ProguardMapping};
 
  #[test]
  fn remap_exception_simple() {
@@ -454,9 +454,12 @@ org.slf4j.helpers.Util$ClassContext -> org.a.b.g$b:
  };
 
  let mapping = ProguardMapping::new(proguard_source);
- let mapper = ProguardMapper::new(mapping);
+ let mut cache = Vec::new();
+ ProguardCache::write(&mapping, &mut cache).unwrap();
+ let cache = ProguardCache::parse(&cache).unwrap();
+ cache.test();
 
- let exception = ProguardService::map_exception(&[&mapper], &exception).unwrap();
+ let exception = ProguardService::map_exception(&[&cache], &exception).unwrap();
 
  assert_eq!(exception.ty, "Util$ClassContextSecurityManager");
  assert_eq!(exception.module, "org.slf4j.helpers");
@@ -534,11 +537,14 @@ io.sentry.sample.MainActivity -> io.sentry.sample.MainActivity:
  ];
 
  let mapping = ProguardMapping::new(proguard_source);
- let mapper = ProguardMapper::new_with_param_mapping(mapping, true);
+ let mut cache = Vec::new();
+ ProguardCache::write(&mapping, &mut cache).unwrap();
+ let cache = ProguardCache::parse(&cache).unwrap();
+ cache.test();
 
  let mapped_frames: Vec<_> = frames
  .iter()
- .flat_map(|frame| ProguardService::map_frame(&[&mapper], frame, None).into_iter())
+ .flat_map(|frame| ProguardService::map_frame(&[&cache], frame, None).into_iter())
  .collect();
 
  assert_eq!(mapped_frames.len(), 7);
@@ -605,7 +611,10 @@ org.slf4j.helpers.Util$ClassContext -> org.a.b.g$b:
 ";
 
  let mapping = ProguardMapping::new(proguard_source);
- let mapper = ProguardMapper::new(mapping);
+ let mut cache = Vec::new();
+ ProguardCache::write(&mapping, &mut cache).unwrap();
+ let cache = ProguardCache::parse(&cache).unwrap();
+ cache.test();
 
  let frames = [
  JvmFrame {
@@ -645,7 +654,7 @@ org.slf4j.helpers.Util$ClassContext -> org.a.b.g$b:
  let mapped_frames: Vec<_> = frames
  .iter()
  .flat_map(|frame| {
- ProguardService::map_frame(&[&mapper], frame, Some("org.slf4j")).into_iter()
+ ProguardService::map_frame(&[&cache], frame, Some("org.slf4j")).into_iter()
  })
  .collect();
 
@@ -702,7 +711,10 @@ org.slf4j.helpers.Util$ClassContext -> org.a.b.g$b:
 "#;
 
  let mapping = ProguardMapping::new(proguard_source);
- let mapper = ProguardMapper::new(mapping);
+ let mut cache = Vec::new();
+ ProguardCache::write(&mapping, &mut cache).unwrap();
+ let cache = ProguardCache::parse(&cache).unwrap();
+ cache.test();
 
  let frame = JvmFrame {
  function: "onCreate".into(),
@@ -713,7 +725,7 @@ org.slf4j.helpers.Util$ClassContext -> org.a.b.g$b:
  ..Default::default()
  };
 
- let mapped_frames = ProguardService::map_frame(&[&mapper], &frame, None);
+ let mapped_frames = ProguardService::map_frame(&[&cache], &frame, None);
 
  assert_eq!(mapped_frames.len(), 2);
 
@@ -769,7 +781,10 @@ y.b -> y.b:
 "#;
 
  let mapping = ProguardMapping::new(proguard_source);
- let mapper = ProguardMapper::new(mapping);
+ let mut cache = Vec::new();
+ ProguardCache::write(&mapping, &mut cache).unwrap();
+ let cache = ProguardCache::parse(&cache).unwrap();
+ cache.test();
 
  let frame = JvmFrame {
  function: "run".into(),
@@ -780,7 +795,7 @@ y.b -> y.b:
  ..Default::default()
  };
 
- let mapped_frames = ProguardService::map_frame(&[&mapper], &frame, None);
+ let mapped_frames = ProguardService::map_frame(&[&cache], &frame, None);
 
  assert_eq!(mapped_frames.len(), 1);
 
@@ -858,7 +873,10 @@ io.wzieba.r8fullmoderenamessources.R -> a.d:
  let mapping = ProguardMapping::new(proguard_source);
  assert!(mapping.is_valid());
  assert!(mapping.has_line_info());
- let mapper = ProguardMapper::new(mapping);
+ let mut cache = Vec::new();
+ ProguardCache::write(&mapping, &mut cache).unwrap();
+ let cache = ProguardCache::parse(&cache).unwrap();
+ cache.test();
 
  let frames: Vec<JvmFrame> = serde_json::from_str(
  r#"[{
@@ -916,7 +934,7 @@ io.wzieba.r8fullmoderenamessources.R -> a.d:
 
  let (remapped_filenames, remapped_abs_paths): (Vec<_>, Vec<_>) = frames
  .iter()
- .flat_map(|frame| ProguardService::map_frame(&[&mapper], frame, None).into_iter())
+ .flat_map(|frame| ProguardService::map_frame(&[&cache], frame, None).into_iter())
  .map(|frame| (frame.filename.unwrap(), frame.abs_path.unwrap()))
  .unzip();
 

crates/symbolicator-service/src/caches/versions.rs

@@ -142,7 +142,8 @@ pub const BUNDLE_INDEX_CACHE_VERSIONS: CacheVersions = CacheVersions {
 /// Proguard Cache, with the following versions:
 ///
 /// - `1`: Initial version.
+/// - `2`: Use proguard cache format (<https://github.com/getsentry/symbolicator/pull/1491>).
 pub const PROGUARD_CACHE_VERSIONS: CacheVersions = CacheVersions {
- current: 1,
+ current: 2,
  fallbacks: &[],
 };