Skip to content

Commit

Permalink
feat: Use proguard cache format (#1491)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@loewenheim
loewenheim authored Jul 4, 2024
1 parent 9a581c8 commit bede054
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 55 deletions.
8 changes: 6 additions & 2 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/symbolicator-proguard/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
63 changes: 30 additions & 33 deletions crates/symbolicator-proguard/src/service.rs
View file
Original file line number Diff line number Diff line change
@@ -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,
};
Expand Down Expand Up @@ -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()
Expand All @@ -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.
Expand Down Expand Up @@ -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> {
Expand All @@ -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
}
}

Expand All @@ -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;

Expand All @@ -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)
}
}
54 changes: 36 additions & 18 deletions crates/symbolicator-proguard/src/symbolication.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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>>,
Expand Down Expand Up @@ -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)?;
Expand All @@ -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 {
Expand Down Expand Up @@ -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() {
Expand All @@ -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");
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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(),
Expand All @@ -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);

Expand Down Expand Up @@ -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(),
Expand All @@ -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);

Expand Down Expand Up @@ -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#"[{
Expand Down Expand Up @@ -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();

Expand Down
3 changes: 2 additions & 1 deletion crates/symbolicator-service/src/caches/versions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: &[],
};

0 comments on commit bede054

Please sign in to comment.