Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Differentiate between memory and file cache keys #1487

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crates/symbolicator-js/src/lookup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1136,7 +1136,9 @@ impl ArtifactFetcher {
source: source_content,
sourcemap,
};
self.sourcemap_caches.compute_memoized(req, cache_key).await
self.sourcemap_caches
.compute_memoized(req, cache_key.clone(), cache_key)
.await
}
Err(err) => Err(err),
},
Expand Down
2 changes: 1 addition & 1 deletion crates/symbolicator-native/src/caches/bitcode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ impl BitcodeService {
};
let cache_key = CacheKey::from_scoped_file(&scope, &request.file_source);
self.cache
.compute_memoized(request, cache_key)
.compute_memoized(request, cache_key.clone(), cache_key)
.bind_hub(hub)
});

Expand Down
5 changes: 4 additions & 1 deletion crates/symbolicator-native/src/caches/cficaches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,10 @@ impl CfiCacheActor {
meta_handle,
};
async {
let entry = self.cficaches.compute_memoized(request, cache_key).await;
let entry = self
.cficaches
.compute_memoized(request, cache_key.clone(), cache_key)
.await;

entry.map(|item| item.1)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/symbolicator-native/src/caches/il2cpp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ impl Il2cppService {
download_svc: self.download_svc.clone(),
};
self.cache
.compute_memoized(request, cache_key)
.compute_memoized(request, cache_key.clone(), cache_key)
.bind_hub(hub)
});

Expand Down
3 changes: 2 additions & 1 deletion crates/symbolicator-native/src/caches/ppdb_caches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ impl PortablePdbCacheActor {
objects_actor: self.objects.clone(),
object_meta,
};
self.ppdb_caches.compute_memoized(request, cache_key)
self.ppdb_caches
.compute_memoized(request, cache_key.clone(), cache_key)
})
.await
}
Expand Down
4 changes: 3 additions & 1 deletion crates/symbolicator-native/src/caches/symcaches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,9 @@ impl SymCacheActor {
secondary_sources,
object_meta: Arc::clone(&handle),
};
self.symcaches.compute_memoized(request, cache_key).await
self.symcaches
.compute_memoized(request, cache_key.clone(), cache_key)
.await
})
.await
}
Expand Down
2 changes: 1 addition & 1 deletion crates/symbolicator-proguard/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl ProguardService {
};

self.cache
.compute_memoized(request, cache_key)
.compute_memoized(request, cache_key.clone(), cache_key)
.await
.map(|item| item.1)
}
Expand Down
4 changes: 3 additions & 1 deletion crates/symbolicator-service/src/caches/sourcefiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ impl SourceFilesCache {
use_shared_cache,
};

self.cache.compute_memoized(request, cache_key).await
self.cache
.compute_memoized(request, cache_key.clone(), cache_key)
.await
}
}

Expand Down
24 changes: 19 additions & 5 deletions crates/symbolicator-service/src/caching/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,25 @@ impl<T: CacheItemRequest> Cacher<T> {
/// The computation itself is done by [`T::compute`](CacheItemRequest::compute), but only if it
/// was not already in the cache.
///
/// # `memory_cache_key` vs. `file_cache_key`
///
/// There are two different keys for looking up the results: `memory_cache_key` and
/// `file_cache_key`. As the names suggest, the former is used to look up the result in
/// the in-memory cache and the latter in the filesystem cache. In most cases the two keys
/// will be the same because items in memory and on disk correspond one to one, but we can
/// also account for the case where different in-memory items are derived from the same file
/// on disk.
///
/// # Errors
///
/// Cache computation can fail, in which case [`T::compute`](CacheItemRequest::compute)
/// will return an `Err`. This err may be persisted in the cache for a time.
pub async fn compute_memoized(&self, request: T, cache_key: CacheKey) -> CacheEntry<T::Item> {
pub async fn compute_memoized(
&self,
request: T,
memory_cache_key: CacheKey,
file_cache_key: CacheKey,
) -> CacheEntry<T::Item> {
let name = self.config.name();
metric!(counter("caches.access") += 1, "cache" => name.as_ref());

Expand All @@ -310,7 +324,7 @@ impl<T: CacheItemRequest> Cacher<T> {
&self.config,
shared_cache,
cache_dir,
&cache_key.cache_path(version),
&file_cache_key.cache_path(version),
is_current_version,
) {
Err(CacheError::NotFound) => continue,
Expand All @@ -331,7 +345,7 @@ impl<T: CacheItemRequest> Cacher<T> {
"version" => &version.to_string(),
"cache" => name.as_ref(),
);
self.spawn_refresh(cache_key.clone(), request);
self.spawn_refresh(file_cache_key.clone(), request);
}

return item;
Expand All @@ -343,7 +357,7 @@ impl<T: CacheItemRequest> Cacher<T> {
metric!(counter("caches.file.miss") += 1, "cache" => name.as_ref());

let item = self
.compute(request, &cache_key, false)
.compute(request, &file_cache_key, false)
// NOTE: We have seen this deadlock with an SDK that was deadlocking on
// out-of-order Scope pops.
// To guarantee that this does not happen is really the responsibility of
Expand All @@ -359,7 +373,7 @@ impl<T: CacheItemRequest> Cacher<T> {

let entry = self
.cache
.entry_by_ref(&cache_key)
.entry_by_ref(&memory_cache_key)
.or_insert_with(init)
.await;

Expand Down
24 changes: 18 additions & 6 deletions crates/symbolicator-service/src/caching/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -805,15 +805,21 @@ async fn test_cache_fallback() {
.unwrap();
let cacher = Cacher::new(cache, Default::default());

let first_result = cacher.compute_memoized(request.clone(), key.clone()).await;
let first_result = cacher
.compute_memoized(request.clone(), key.clone(), key.clone())
.await;
assert_eq!(first_result.unwrap().as_str(), "some old cached contents");

let second_result = cacher.compute_memoized(request.clone(), key.clone()).await;
let second_result = cacher
.compute_memoized(request.clone(), key.clone(), key.clone())
.await;
assert_eq!(second_result.unwrap().as_str(), "some old cached contents");

tokio::time::sleep(Duration::from_millis(200)).await;

let third_result = cacher.compute_memoized(request.clone(), key).await;
let third_result = cacher
.compute_memoized(request.clone(), key.clone(), key)
.await;
assert_eq!(third_result.unwrap().as_str(), "some new cached contents");

// we only want to have the actual computation be done a single time
Expand Down Expand Up @@ -854,7 +860,9 @@ async fn test_cache_fallback_notfound() {
.unwrap();
let cacher = Cacher::new(cache, Default::default());

let first_result = cacher.compute_memoized(request.clone(), key).await;
let first_result = cacher
.compute_memoized(request.clone(), key.clone(), key)
.await;
assert_eq!(first_result, Err(CacheError::NotFound));

// no computation should be done
Expand Down Expand Up @@ -892,7 +900,9 @@ async fn test_lazy_computation_limit() {
fs::create_dir_all(cache_file.parent().unwrap()).unwrap();
fs::write(cache_file, "some old cached contents").unwrap();

let result = cacher.compute_memoized(request.clone(), key).await;
let result = cacher
.compute_memoized(request.clone(), key.clone(), key)
.await;
assert_eq!(result.unwrap().as_str(), "some old cached contents");
}

Expand All @@ -909,7 +919,9 @@ async fn test_lazy_computation_limit() {
let request = request.clone();
let key = CacheKey::for_testing(*key);

let result = cacher.compute_memoized(request.clone(), key).await;
let result = cacher
.compute_memoized(request.clone(), key.clone(), key)
.await;
if result.unwrap().as_str() == "some old cached contents" {
num_outdated += 1;
}
Expand Down
6 changes: 5 additions & 1 deletion crates/symbolicator-service/src/objects/meta_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,11 @@ impl FetchFileMetaRequest {

let object_handle = self
.data_cache
.compute_memoized(FetchFileDataRequest(self.clone()), cache_key.clone())
.compute_memoized(
FetchFileDataRequest(self.clone()),
cache_key.clone(),
cache_key.clone(),
)
.await?;

let object = object_handle.object();
Expand Down
9 changes: 7 additions & 2 deletions crates/symbolicator-service/src/objects/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,9 @@ impl ObjectsActor {
download_svc: self.download_svc.clone(),
});

self.data_cache.compute_memoized(request, cache_key).await
self.data_cache
.compute_memoized(request, cache_key.clone(), cache_key)
.await
}

/// Fetches matching objects and returns the metadata of the most suitable object.
Expand Down Expand Up @@ -175,7 +177,10 @@ impl ObjectsActor {
};

async move {
let handle = self.meta_cache.compute_memoized(request, cache_key).await;
let handle = self
.meta_cache
.compute_memoized(request, cache_key.clone(), cache_key)
.await;
FoundMeta {
file_source,
handle,
Expand Down
Loading