Skip to content

Commit 737dc05

Browse files
GealGeal
authored
Revert "use a parking-lot mutex in cache storage (#2887)" (#2980)
This reverts commit c0218a3.
1 parent e691bde commit 737dc05

File tree

7 files changed

+56
-69
lines changed

7 files changed

+56
-69
lines changed

‎.changesets/fix_geal_cache_storage_mutex.md

Lines changed: 0 additions & 5 deletions
This file was deleted.

‎apollo-router/src/cache/mod.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,8 @@ where
124124
let _ = sender.send(value);
125125
}
126126

127-
pub(crate) fn in_memory_keys(&self) -> Vec<K> {
128-
self.storage.in_memory_keys()
127+
pub(crate) async fn in_memory_keys(&self) -> Vec<K> {
128+
self.storage.in_memory_keys().await
129129
}
130130
}
131131

@@ -235,7 +235,7 @@ mod tests {
235235
entry.insert(i).await;
236236
}
237237

238-
assert_eq!(cache.storage.len(), 13);
238+
assert_eq!(cache.storage.len().await, 13);
239239
}
240240

241241
mock! {
@@ -271,6 +271,6 @@ mod tests {
271271
while let Some(_result) = computations.next().await {}
272272

273273
// To be really sure, check there is only one value in the cache
274-
assert_eq!(cache.storage.len(), 1);
274+
assert_eq!(cache.storage.len().await, 1);
275275
}
276276
}

‎apollo-router/src/cache/storage.rs

Lines changed: 43 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@ use std::num::NonZeroUsize;
77
use std::sync::Arc;
88

99
use lru::LruCache;
10-
use parking_lot::Mutex;
1110
use serde::de::DeserializeOwned;
1211
use serde::Serialize;
12+
use tokio::sync::Mutex;
1313
use tokio::time::Instant;
1414

1515
use super::redis::*;
@@ -84,15 +84,42 @@ where
8484
}
8585

8686
pub(crate) async fn get(&self, key: &K) -> Option<V> {
87-
match self.get_in_memory(key) {
88-
Some(v) => Some(v),
87+
let mut guard = self.inner.lock().await;
88+
let instant_memory = Instant::now();
89+
match guard.get(key) {
90+
Some(v) => {
91+
tracing::info!(
92+
monotonic_counter.apollo_router_cache_hit_count = 1u64,
93+
kind = %self.caller,
94+
storage = &tracing::field::display(CacheStorageName::Memory),
95+
);
96+
let duration = instant_memory.elapsed().as_secs_f64();
97+
tracing::info!(
98+
histogram.apollo_router_cache_hit_time = duration,
99+
kind = %self.caller,
100+
storage = &tracing::field::display(CacheStorageName::Memory),
101+
);
102+
Some(v.clone())
103+
}
89104
None => {
105+
let duration = instant_memory.elapsed().as_secs_f64();
106+
tracing::info!(
107+
histogram.apollo_router_cache_miss_time = duration,
108+
kind = %self.caller,
109+
storage = &tracing::field::display(CacheStorageName::Memory),
110+
);
111+
tracing::info!(
112+
monotonic_counter.apollo_router_cache_miss_count = 1u64,
113+
kind = %self.caller,
114+
storage = &tracing::field::display(CacheStorageName::Memory),
115+
);
116+
90117
let instant_redis = Instant::now();
91118
if let Some(redis) = self.redis.as_ref() {
92119
let inner_key = RedisKey(key.clone());
93120
match redis.get::<K, V>(inner_key).await {
94121
Some(v) => {
95-
self.insert_in_memory(key.clone(), v.0.clone());
122+
guard.put(key.clone(), v.0.clone());
96123
tracing::info!(
97124
monotonic_counter.apollo_router_cache_hit_count = 1u64,
98125
kind = %self.caller,
@@ -128,71 +155,35 @@ where
128155
}
129156
}
130157

131-
fn get_in_memory(&self, key: &K) -> Option<V> {
132-
let instant_memory = Instant::now();
133-
134-
match self.inner.lock().get(key).cloned() {
135-
Some(v) => {
136-
tracing::info!(
137-
monotonic_counter.apollo_router_cache_hit_count = 1u64,
138-
kind = %self.caller,
139-
storage = &tracing::field::display(CacheStorageName::Memory),
140-
);
141-
let duration = instant_memory.elapsed().as_secs_f64();
142-
tracing::info!(
143-
histogram.apollo_router_cache_hit_time = duration,
144-
kind = %self.caller,
145-
storage = &tracing::field::display(CacheStorageName::Memory),
146-
);
147-
Some(v)
148-
}
149-
None => {
150-
let duration = instant_memory.elapsed().as_secs_f64();
151-
tracing::info!(
152-
histogram.apollo_router_cache_miss_time = duration,
153-
kind = %self.caller,
154-
storage = &tracing::field::display(CacheStorageName::Memory),
155-
);
156-
tracing::info!(
157-
monotonic_counter.apollo_router_cache_miss_count = 1u64,
158-
kind = %self.caller,
159-
storage = &tracing::field::display(CacheStorageName::Memory),
160-
);
161-
None
162-
}
163-
}
164-
}
165-
166158
pub(crate) async fn insert(&self, key: K, value: V) {
167159
if let Some(redis) = self.redis.as_ref() {
168160
redis
169161
.insert(RedisKey(key.clone()), RedisValue(value.clone()))
170162
.await;
171163
}
172164

173-
self.insert_in_memory(key, value);
174-
}
175-
176-
fn insert_in_memory(&self, key: K, value: V) {
177-
let size = {
178-
let mut in_memory = self.inner.lock();
179-
in_memory.put(key, value);
180-
in_memory.len() as u64
181-
};
165+
let mut in_memory = self.inner.lock().await;
166+
in_memory.put(key, value);
167+
let size = in_memory.len() as u64;
182168
tracing::info!(
183169
value.apollo_router_cache_size = size,
184170
kind = %self.caller,
185171
storage = &tracing::field::display(CacheStorageName::Memory),
186172
);
187173
}
188174

189-
pub(crate) fn in_memory_keys(&self) -> Vec<K> {
190-
self.inner.lock().iter().map(|(k, _)| k.clone()).collect()
175+
pub(crate) async fn in_memory_keys(&self) -> Vec<K> {
176+
self.inner
177+
.lock()
178+
.await
179+
.iter()
180+
.map(|(k, _)| k.clone())
181+
.collect()
191182
}
192183

193184
#[cfg(test)]
194-
pub(crate) fn len(&self) -> usize {
195-
self.inner.lock().len()
185+
pub(crate) async fn len(&self) -> usize {
186+
self.inner.lock().await.len()
196187
}
197188
}
198189

‎apollo-router/src/query_planner/caching_query_planner.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,8 @@ where
5858
}
5959
}
6060

61-
pub(crate) fn cache_keys(&self, count: usize) -> Vec<(String, Option<String>)> {
62-
let keys = self.cache.in_memory_keys();
61+
pub(crate) async fn cache_keys(&self, count: usize) -> Vec<(String, Option<String>)> {
62+
let keys = self.cache.in_memory_keys().await;
6363
keys.into_iter()
6464
.take(count)
6565
.map(|key| (key.query, key.operation))

‎apollo-router/src/router_factory.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -211,8 +211,9 @@ impl RouterSuperServiceFactory for YamlRouterFactory {
211211

212212
if let Some(router) = previous_router {
213213
if configuration.supergraph.query_planning.warmed_up_queries > 0 {
214-
let cache_keys =
215-
router.cache_keys(configuration.supergraph.query_planning.warmed_up_queries);
214+
let cache_keys = router
215+
.cache_keys(configuration.supergraph.query_planning.warmed_up_queries)
216+
.await;
216217

217218
if !cache_keys.is_empty() {
218219
tracing::info!(

‎apollo-router/src/services/router_service.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -515,8 +515,8 @@ where
515515
}
516516

517517
impl RouterCreator<crate::services::supergraph_service::SupergraphCreator> {
518-
pub(crate) fn cache_keys(&self, count: usize) -> Vec<(String, Option<String>)> {
519-
self.supergraph_creator.cache_keys(count)
518+
pub(crate) async fn cache_keys(&self, count: usize) -> Vec<(String, Option<String>)> {
519+
self.supergraph_creator.cache_keys(count).await
520520
}
521521

522522
pub(crate) fn planner(&self) -> Arc<Planner<QueryPlanResult>> {

‎apollo-router/src/services/supergraph_service.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -442,8 +442,8 @@ impl SupergraphCreator {
442442
)
443443
}
444444

445-
pub(crate) fn cache_keys(&self, count: usize) -> Vec<(String, Option<String>)> {
446-
self.query_planner_service.cache_keys(count)
445+
pub(crate) async fn cache_keys(&self, count: usize) -> Vec<(String, Option<String>)> {
446+
self.query_planner_service.cache_keys(count).await
447447
}
448448

449449
pub(crate) fn planner(&self) -> Arc<Planner<QueryPlanResult>> {

0 commit comments

Comments
 (0)