*: Address review on cache improvements

Author: leoyvens

graph/src/data/query/result.rs

@@ -57,6 +57,10 @@ impl QueryResult {
     }
 
     pub fn append(&mut self, mut other: QueryResult) {
+        // Currently we don't used extensions, the desired behaviour for merging them is tbd.
+        assert!(self.extensions.is_none());
+        assert!(other.extensions.is_none());
+
         match (&mut self.data, &mut other.data) {
             (Some(q::Value::Object(ours)), Some(q::Value::Object(other))) => ours.append(other),
 
@@ -73,10 +77,6 @@ impl QueryResult {
             // Only one side has errors, use that.
             _ => self.errors = self.errors.take().or(other.errors),
         }
-
-        // Currently we don't used extensions, the desired behaviour for merging them is tbd.
-        assert!(self.extensions.is_none());
-        assert!(other.extensions.is_none());
     }
 }
 

graph/src/data/store/mod.rs

@@ -1,5 +1,5 @@
 use crate::data::subgraph::SubgraphDeploymentId;
-use crate::prelude::{format_err, EntityKey, QueryExecutionError};
+use crate::prelude::{format_err, CacheWeight, EntityKey, QueryExecutionError};
 use failure::Error;
 use graphql_parser::query;
 use graphql_parser::schema;
@@ -580,6 +580,12 @@ impl<'a> From<Vec<(&'a str, Value)>> for Entity {
     }
 }
 
+impl CacheWeight for Entity {
+    fn indirect_weight(&self) -> usize {
+        self.0.indirect_weight()
+    }
+}
+
 /// A value that can (maybe) be converted to an `Entity`.
 pub trait TryIntoEntity {
     fn try_into_entity(self) -> Result<Entity, Error>;

graph/src/util/cache_weight.rs

@@ -1,11 +1,12 @@
-use crate::prelude::{BigDecimal, BigInt, Entity, Value};
+use crate::prelude::{BigDecimal, BigInt, Value};
+use std::mem;
 
 /// Estimate of how much memory a value consumes.
 /// Useful for measuring the size of caches.
 pub trait CacheWeight {
     /// Total weight of the value.
     fn weight(&self) -> usize {
-        std::mem::size_of_val(&self) + self.indirect_weight()
+        mem::size_of_val(&self) + self.indirect_weight()
     }
 
     /// The weight of values pointed to by this value but logically owned by it, which is not
@@ -24,7 +25,8 @@ impl<T: CacheWeight> CacheWeight for Option<T> {
 
 impl<T: CacheWeight> CacheWeight for Vec<T> {
     fn indirect_weight(&self) -> usize {
-        self.iter().map(CacheWeight::indirect_weight).sum()
+        self.iter().map(CacheWeight::indirect_weight).sum::<usize>()
+            + self.capacity() * mem::size_of::<T>()
     }
 }
 
@@ -36,15 +38,18 @@ impl<T: CacheWeight, U: CacheWeight> CacheWeight for std::collections::BTreeMap<
     }
 }
 
-impl CacheWeight for &'_ [u8] {
+impl<T: CacheWeight, U: CacheWeight> CacheWeight for std::collections::HashMap<T, U> {
     fn indirect_weight(&self) -> usize {
-        self.len()
+        self.iter()
+            .map(|(key, value)| key.indirect_weight() + value.indirect_weight())
+            .sum::<usize>()
+            + self.capacity() * mem::size_of::<T>()
     }
 }
 
 impl CacheWeight for String {
     fn indirect_weight(&self) -> usize {
-        self.len()
+        self.capacity()
     }
 }
 
@@ -60,27 +65,25 @@ impl CacheWeight for BigInt {
     }
 }
 
+impl CacheWeight for crate::data::store::scalar::Bytes {
+    fn indirect_weight(&self) -> usize {
+        self.as_slice().len()
+    }
+}
+
 impl CacheWeight for Value {
     fn indirect_weight(&self) -> usize {
         match self {
             Value::String(s) => s.indirect_weight(),
             Value::BigDecimal(d) => d.indirect_weight(),
             Value::List(values) => values.indirect_weight(),
-            Value::Bytes(bytes) => bytes.as_slice().indirect_weight(),
+            Value::Bytes(bytes) => bytes.indirect_weight(),
             Value::BigInt(n) => n.indirect_weight(),
             Value::Int(_) | Value::Bool(_) | Value::Null => 0,
         }
     }
 }
 
-impl CacheWeight for Entity {
-    fn indirect_weight(&self) -> usize {
-        self.iter()
-            .map(|(key, value)| key.weight() + value.weight())
-            .sum()
-    }
-}
-
 impl CacheWeight for graphql_parser::query::Value {
     fn indirect_weight(&self) -> usize {
         use graphql_parser::query as q;

graphql/src/execution/cache.rs

@@ -9,11 +9,9 @@ use std::sync::{Arc, Condvar, Mutex};
 type Hash = <SetHasher as StableHasher>::Out;
 
 /// The 'true' cache entry that lives inside the Arc.
-/// When the last Arc is dropped, this is dropped,
-/// and the cache is removed.
+/// When the last Arc is dropped, this is dropped, and the cache is removed.
 #[derive(Debug)]
 struct CacheEntryInner<R> {
-    hash: Hash,
     // Considered using once_cell::sync::Lazy,
     // but that quickly becomes a mess of generics
     // or runs into the issue that Box<dyn FnOnce> can't be
@@ -26,9 +24,8 @@ struct CacheEntryInner<R> {
 }
 
 impl<R> CacheEntryInner<R> {
-    fn new(hash: Hash) -> Arc<Self> {
+    fn new() -> Arc<Self> {
         Arc::new(Self {
-            hash,
             result: OnceCell::new(),
             condvar: Condvar::new(),
             lock: Mutex::new(false),
@@ -134,7 +131,7 @@ impl<R: CheapClone> QueryCache<R> {
                     return entry.wait().cheap_clone();
                 }
                 Entry::Vacant(entry) => {
-                    let uncached = CacheEntryInner::new(hash);
+                    let uncached = CacheEntryInner::new();
                     entry.insert(uncached.clone());
                     uncached
                 }

server/http/src/response.rs

@@ -100,7 +100,7 @@ mod tests {
     #[test]
     fn generates_401_for_query_errors() {
         let parse_error = graphql_parser::parse_query("<>?><").unwrap_err();
-        let query_error = QueryError::ParseError(parse_error.into());
+        let query_error = QueryError::ParseError(Arc::new(parse_error.compat().into()));
         let future = GraphQLResponse::new(Err(GraphQLServerError::from(query_error)));
         let response = future.wait().expect("Should generate a response");
         test_utils::assert_error_response(response, StatusCode::BAD_REQUEST);
@@ -170,7 +170,7 @@ mod tests {
     fn generates_valid_json_for_query_error() {
         let parse_error =
             graphql_parser::parse_query("<><?").expect_err("Should fail parsing an invalid query");
-        let query_error = QueryError::ParseError(parse_error.into());
+        let query_error = QueryError::ParseError(Arc::new(parse_error.compat().into()));
         let err = GraphQLServerError::QueryError(query_error);
         let future = GraphQLResponse::new(Err(err));
         let response = future.wait().expect("Should generate a response");