Ayende @ Rahien

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Sun, 28 Aug 2011 18:48:29 GMT

Hendry, You don't need locking to do that. And the issues isn't of concurrent request, the issue is of concurrent _system_. 2 concurrent requests would make 2 separate requests, most probably, nothing wrong there. But you have to account for concurrency in the design because that is where it will run.

Hendry Luk commented on Elegancy challenge: Cacheable Batches

Sun, 28 Aug 2011 16:51:28 GMT

@Mark, regarding list/array optimization, I think Linq would be most efficient solution. E.g. ids.Select(GetDocument).ToArray(); @Ayende, regarding thread-safetiness, why does that matter. Why is it important that 2 concurrent requests with the same id MUST make exactly 1 request? I think the performance overhead to ensure this requirement (i.e. locking) would outweigh its gain.

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Fri, 26 Aug 2011 10:18:07 GMT

Olcay, Take a look at the RaccoonBlog code on github :-)

Olcay Şeker commented on Elegancy challenge: Cacheable Batches

Fri, 26 Aug 2011 10:16:33 GMT

@Ayende, can we see your solution about this challenge?

Mark commented on Elegancy challenge: Cacheable Batches

Thu, 25 Aug 2011 17:30:17 GMT

Matt: I am using GetValues.

Matt Warren commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 15:08:21 GMT

I think that only applies if you're using the GetValues(..) method, but not if you're using the Indexer or Item property (see http://msdn.microsoft.com/en-us/library/system.runtime.caching.memorycache.item.aspx). Either way it's a small detail and I like the your solution

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 14:22:51 GMT

Matt, MemoryCache returns all the keys in the Dictionary, with nulls where there was no matching value.

Matt Warren commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 13:32:05 GMT

Mark, 1 small bug, the line foreach (var value in _uncachedResolver(keys.Where(k => values[k] == null))) should be foreach (var value in _uncachedResolver(keys.Where(k => !values.ContainsKey(k)))) Otherwise it throws an exception when the key isn't in a cache rather than fetching it.

Jesse commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 13:00:17 GMT

I rather like Mark's solution.

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 11:16:41 GMT

I've posted that class along with how it would be used by ClientProxy as a gist: https://gist.github.com/1167813

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 11:05:20 GMT

Yes, I forgot to fix that check line to account for Nulls in the dictionary that comes back from the cache. So that line should be if (values.Any(kvp => kvp.Value == null)) and done. Interesting to note that if you post enough comments, the Captcha stops appearing. Nice UX.

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:59:28 GMT

Mark, That is pretty bad, yes. Mostly because unless managed properly, caches are actually memory leaks :-) No worries for this scenario, but you might want to check how much trouble that got us with the RavenDB caching

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:58:31 GMT

Mark, Oh! Good point, I didn't see that, I am sorry. Last thing to fix is what to do when everything is cached, you don't need to call the server then.

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:57:28 GMT

Also, I'm probably doing something hideous with my creation of the MemoryCache class with a GUID as the name. I suspect a better way would be to use a named region in the Default cache.

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:53:41 GMT

While I'm learning about MemoryCache, which is great, more and more requirements are rearing their heads here. class ElegantCache : IDisposable { private readonly MemoryCache _cache = new MemoryCache(Guid.NewGuid().ToString()); private readonly CacheItemPolicy _cacheItemPolicy; private readonly Func _keyResolver; private readonly Func, IEnumerable> _uncachedResolver; public ElegantCache(CacheItemPolicy cacheItemPolicy, Func keyResolver, Func, IEnumerable> uncachedResolver) { _cacheItemPolicy = cacheItemPolicy; _keyResolver = keyResolver; _uncachedResolver = uncachedResolver; } public IEnumerable Get(string[] keys) { var values = _cache.GetValues(keys) ?? new Dictionary(); if (values.Count < keys.Length) { foreach (var value in _uncachedResolver(keys.Where(k => values[k] == null))) { var key = _keyResolver(value); values[key] = value; _cache.Set(key, value, _cacheItemPolicy); } } return keys.Select(key => values[key]).Cast(); } public void Dispose() { _cache.Dispose(); } } I'm not making multiple DB calls, the _uncachedResolver function takes a list of strings and returns a list of values. The function would look like: ids => { var request = WebRequest.Create("http://server/get?id=" + string.Join("&id=", ids)); using (var stream = request.GetResponse().GetResponseStream()) { return GetResults(stream); } }

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:44:15 GMT

Mark, I apologize, my (unvoiced) assumption was that the cache lifetime is longer than the lifetime of the proxy Good catch with regards to the server code

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:41:00 GMT

Ayende, Your use case for the proxy showed it being used and disposed in a very short space of time, so memory leakage, thread-safety and performance around the overall size of the cache were not an obvious part of your requirements. You simplified the problem drastically, I simplified the solution likewise. While we're in code review mode, your server method is inefficient in terms of collection management. You should create the List with the known size to prevent array copies as the list resizes its internals, or, even better, just create the array and then use a for loop to populate it: public JsonDocument[] GetDocument(string[] ids) { var results = new JsonDocument[ids.Length]; for(int i = 0; i < ids.Length; i++) { results[i] = GetDocument(ids[i]); } return results; }

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:40:45 GMT

Mark, In the second version, same problem in that GetValues return all keys, even not on the cache. You are still making N calls to the DB. Your call to AddOrGetExisting will get an existing (potentially older) value that has been already set in the cache. You should prefer to use the value from the server

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:39:06 GMT

Mark, a) The GetValues will give you all the keys back (with the value set to null), so the code would fail. b) You are resolving those items one at a time, not all at once, which means that you just broke batching.

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:31:05 GMT

Non-leaking, more performant, thread-safe... class ElegantCache : IDisposable { private readonly MemoryCache _cache = new MemoryCache(Guid.NewGuid().ToString()); private readonly CacheItemPolicy _cacheItemPolicy; private readonly Func _keyResolver; private readonly Func, IEnumerable> _uncachedResolver; public ElegantCache(CacheItemPolicy cacheItemPolicy, Func keyResolver, Func, IEnumerable> uncachedResolver) { _cacheItemPolicy = cacheItemPolicy; _keyResolver = keyResolver; _uncachedResolver = uncachedResolver; } public IEnumerable Get(string[] keys) { var values = _cache.GetValues(keys) ?? new Dictionary(); if (values.Count < keys.Length) { foreach (var value in _uncachedResolver(keys.Where(k => !values.ContainsKey(k)))) { var key = _keyResolver(value); values.Add(key, _cache.AddOrGetExisting(key, value, _cacheItemPolicy)); } } return keys.Select(key => values[key]).Cast(); } public void Dispose() { _cache.Dispose(); } }

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:25:16 GMT

Non-leaking version: class ElegantCache : IDisposable { private readonly MemoryCache _cache = new MemoryCache(Guid.NewGuid().ToString()); private readonly CacheItemPolicy _cacheItemPolicy; private readonly Func _keyResolver; private readonly Func, IEnumerable> _uncachedResolver; public ElegantCache(CacheItemPolicy cacheItemPolicy, Func keyResolver, Func, IEnumerable> uncachedResolver) { _cacheItemPolicy = cacheItemPolicy; _keyResolver = keyResolver; _uncachedResolver = uncachedResolver; } public IEnumerable Get(string[] keys) { var values = _cache.GetValues(keys) ?? new Dictionary(); if (values.Count < keys.Length) { foreach (var value in _uncachedResolver(keys.Except(values.Keys))) { values.Add(_keyResolver(value), value); _cache.Add(_keyResolver(value), value, _cacheItemPolicy); } } return keys.Select(key => values[key]).Cast(); } public void Dispose() { _cache.Dispose(); } }

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:17:46 GMT

Mark, Aside from everything else: keys.Except(_cache.Keys) Is probably much more expensive than doing a dictionary lookup for each key, since you are forcing iteration of the entire thing, which is very expensive for large number of items. Same issue regarding the memory leak as before. Also, not safe for multi threading

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:12:56 GMT

Revised: keys.Join line was abominable. class ElegantCache { private readonly Dictionary _cache = new Dictionary(); private readonly Func _keyResolver; private readonly Func, IEnumerable> _uncachedResolver; public ElegantCache(Func keyResolver, Func, IEnumerable> uncachedResolver) { _keyResolver = keyResolver; _uncachedResolver = uncachedResolver; } public IEnumerable Get(TKey[] keys) { var uncachedKeys = keys.Except(_cache.Keys).ToList(); if (uncachedKeys.Count > 0) { foreach (var value in _uncachedResolver(uncachedKeys)) { _cache.Add(_keyResolver(value), value); } } return keys.Select(key => _cache[key]); } }

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:12:52 GMT

Mark, You don't have a cache, you have a memory leak, since nothing ever expires from this cache

Ayende Rahien commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:10:51 GMT

Jesper, This code suffers from a multi threading bug. We first check if an item exists, and only later we extract it, by that time, it might have been removed from the cache. Same for setting the item in the cache and then trying to get it from there.

Mark commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 10:01:39 GMT

class ElegantCache { private readonly Dictionary _cache = new Dictionary(); private readonly Func _keyResolver; private readonly Func, IEnumerable> _uncachedResolver; public ElegantCache(Func keyResolver, Func, IEnumerable> uncachedResolver) { _keyResolver = keyResolver; _uncachedResolver = uncachedResolver; } public IEnumerable Get(TKey[] keys) { var uncachedKeys = keys.Except(_cache.Keys).ToList(); if (uncachedKeys.Count > 0) { foreach (var value in _uncachedResolver(uncachedKeys)) { _cache.Add(_keyResolver(value), value); } } return keys.Join(_cache, k => k, kvp => kvp.Key, (k, kvp) => kvp.Value); } }

Jesper Larsen-Ledet commented on Elegancy challenge: Cacheable Batches

Wed, 24 Aug 2011 09:54:49 GMT

public JsonDocument[] GetDocument(params string[] ids) { // only ask server for uncached documents var uncachedIds = (from id in ids where !cache.Contains(id) select id).ToArray(); // make the request to the server, for example var request = WebRequest.Create("http://server/get?id=" + string.Join("&id=", uncachedIds)); using (var stream = request.GetResponse().GetResponseStream()) { var results = GetResults(stream); // add fetched documents to cache foreach (var x in uncachedIds.Zip(results, (id, doc) => new { id, doc })) { cache[x.id] = x.doc; } } // return all documents form cache return (from id in ids select (JsonDocument)cache[id]).ToArray(); }