Bug in Caching code

[GoogleCodeExporter]

The "localCache" coad in CacheServiceImpl.java is not correct.  The issues I 
see are:

1. Not thread safe
2. Would potentially have stability issues - e.g. high load across many 
different pages filling up cache
3. Cache may not clear due to volatility of localCacheTime

My recommendation would be too either consider dumping the local cache (Is 
memcache not fast enough?), or if there is a proven benefit, consider using a 
threadsafe expiring or limited size cache.

Here is an example using Google Collectins API (reimplementing your current 5 
second TTL unlimited size cache):

localCache= new MapMaker()
   .concurrencyLevel(32)
   .softValues()
   .expiration(5, TimeUnit.SECONDS).makeMap();

A few points to consider:
 - why 5 seconds?
 - would a fixed size LRUMap do a better job? ( http://commons.apache.org/collections/apidocs/org/apache/commons/collections/LRUMap.html )
 - does appstat so local caching has advantages?

Hope this information is useful. 

For a day job I do programming at PaperCut (<a href="http://www.papercut.com/"> 
http://www.papercut.com/ </a> and were looking at Vosao for my hobby - my 
sailing club website.  If you need a hand with this change or anything else on 
the project, let me know and I'll see if I can spare some time.  Don't mind 
consuming a few work hours... but not the sailing hours :-)

Original issue reported on code.google.com by dance.ch...@gmail.com on 5 Aug 2010 at 12:15

[GoogleCodeExporter]

|1. Not thread safe

App Engine doesn't use threading at all. Every instance runs one thread at a 
time only.

|2. Would potentially have stability issues - e.g. high load across many 
different |pages filling up cache

That is why there is 5 seconds limit.

|3. Cache may not clear due to volatility of localCacheTime

Please give more details.

The benefit of using localCache are timings. Memcache call takes about 7ms but 
localCache are nanoseconds.

Vosao uses two layer cache to access data in GAE.

localCache - nanoseconds
memcache - 7 milliseconds
datastore query - 300ms

Original comment by kinyelo@gmail.com on 5 Aug 2010 at 1:13

[GoogleCodeExporter]

>> App Engine doesn't use threading at all. Every instance runs one thread at a 
time only.

As far as I know is that you're not allowed to spawn a thread in the Appengine 
sandbox, but this doesn't mean, that every instance runs only one thread.
For example every servlet request has a corresponding thread.
Or do you have on official link there this is stated?

Original comment by axelclk@gmail.com on 5 Aug 2010 at 5:07

[GoogleCodeExporter]

1. As per axelclk's comment.  GAE prevents _new_ threads from being started in 
a given request, but this does not mean that two separate requests (e.g. two 
users hitting a same URL at the same time) do not run in parallel in two 
separate threads.  GAE is generally a very "thread safe" environment as use of 
shared data is encouraged via memcache and datastore.  Local static instances 
however needs to be treated with care.

2. I should have said "scalability".  Consider a situation where 500 users hit 
500 different pages within the five second window.  This would load 500 pages 
into the hash.  Not likely to happen on most normal sites, but the thought 
experiment does show the problem with the current strategy.

3. localCacheTime should be declared "volitile" or guarded by a synchronize 
lock, or wrapped in an atomic holder object to ensure all threads see the same 
value.  I however suggest changing cache strategy/algorithm altogether.

I don't have any data (e.g. AppStat traces) to show either way whether using 
local caching for this type of use-case is effective or not.  Assuming it is, 
I'm suggesting changing the code to:
     * fix the thread safety issues
     * reconsider the caching algorithm

Writing a good cache for this type of use-case is quite hard.  A fixed time 
window (current implementation) is not very efficient.  Consider a case where a 
page is cached only 1ms before the window ends - work done for no return.  A 
least-recently-used algorithm (e.g. LRUMap) would be better but still not 
ideal.  Consider a limited cache set at max 10 pages, and someone browse 10 
unpopular pages in quick succession.  This would flush out all popular pages.  
The correct caching algorithm for this use case is some fancy time weighted 
thingy - a topic I'll leave for the academics... or to memcache :-)

Original comment by dance.ch...@gmail.com on 6 Aug 2010 at 12:55

[GoogleCodeExporter]

App Engine apps run in a single-threaded environment. That is, a given
interpreter instance will only ever be processing one request at any given
time. Thus, you don't need to worry about concurrent invocations.

-Nick Johnson

http://www.mail-archive.com/google-appengine@googlegroups.com/msg15383.html

Original comment by kinyelo@gmail.com on 6 Aug 2010 at 7:55

[GoogleCodeExporter]

I agree within this statement for "interpreted" python for which this link is 
referring too. What about Java? 

Original comment by dance.ch...@gmail.com on 6 Aug 2010 at 11:26

[GoogleCodeExporter]

Cache service is never concurrently accessed because it resides in -> 
SystemService which resides in -> Business which resides in VosaoContext which 
is thread local variable. Not singleton!

Original comment by kinyelo@gmail.com on 7 Aug 2010 at 6:45

[GoogleCodeExporter]

You seem right about concurrent requests. I found this thread which is an 
interesting read. 

http://groups.google.com/group/google-appengine-java/browse_thread/thread/2b1374
c45cb795c4/d6b8d7734a392ea5?lnk=gst&q=Jvm+thread+concurrent#d6b8d7734a392ea5

The suggestion regarding caching algorithm still applies.  

Original comment by dance.ch...@gmail.com on 7 Aug 2010 at 10:47

[GoogleCodeExporter]

LocalCache and memcache are used not only for caching rendered pages but also 
to cache  datastore get and query requests. 

5 sec is the maximum time window when you can get stale data. Yes it's not 
perfect it has drawbacks but it helps to reduce content delivery time.

Finally we can set it to 0 and use local cache only during one request.

Original comment by kinyelo@gmail.com on 7 Aug 2010 at 2:15

[GoogleCodeExporter]

Original comment by kinyelo@gmail.com on 13 Aug 2010 at 7:38

• Changed state: WontFix

[GoogleCodeExporter]

In light of the recently announced <threadsafe> setting in appengine-web.xml 
(added in 1.4.3) is this issue worth reconsidering?

Original comment by dance.ch...@gmail.com on 10 Apr 2011 at 7:26

[GoogleCodeExporter]

In 0.9 release I've set <threadsafe> to true. I will work as all resides in 
thread local vars.

Original comment by kinyelo@gmail.com on 10 Apr 2011 at 12:49