Magnolia cache

[MakkaPakka]

Hi folks,

I've got some questions about the cache and hoped you wouldn't mind me harassing you about it. I've some queries around the cache implementation within Magnolia and was hoping you could set me straight. 


I've recently implemented a custom cache policy and as part of that work stumbled upon this piece of code within the info.magnolia.module.cache.cachepolicy.Default class. 


// we need to synchronize on the cache instance, as multiple threads might be accessing this
// concurrently, and we don't want to block the system if we're using a blocking cache.
// (since hasElement() might place a mutex on the cache key)
synchronized (cache) {
    if (cache.hasElement(key)) { 
        final Object cachedEntry = cache.get(key); 
        return new CachePolicyResult(CachePolicyResult.useCache, key, cachedEntry); 
    } else { 
        return new CachePolicyResult(CachePolicyResult.store, key, null); 
    } 

}

1) My first query is around the two calls to retrieve any given element. eg you first call hasElement and then get. Could we not just call get and check null as below?

final Object cachedEntry = cache.get(key);
if (cachedEntry != null) {
      return new CachePolicyResult(CachePolicyResult.useCache, key, cachedEntry); 
} else {
      return new CachePolicyResult(CachePolicyResult.store, key, null); 
}



2) The second query I have is about the use of the synchronised block around the cache retrieval. I've searched the entire codebase and all of the calls which add to the cache have no such synchronized block, only the retrieval does (I could easily have missed them though as I don't have the full source).

The comment above the block doesn't make sense as by synchronising on the cache object you're only marshalling access to that section of code; subsequent calls to hasElement and get will block globally regardless of this?

Under the covers, it appears to be using ehCache which is fully thread safe and no doubt uses appropriate read/write locking.

I would also expect locking code to be encapsulated within the cache class implementation, the code which uses the cache can then be properly agnostic regarding the locking strategy.

Thanks for any pointers you can give.

Luke

[Jan Haderka-2]

It's been a while, so bear with me.
I guess the comment should have read BlockingCache or even better the
net.st.ehcache.construct.blockingBlockingCache

As you have noticed Magnolia uses EhCache underneath. The EhCache
provides 2 implementations of EhCache interface, one of which is
blocking (in a sense that after first call to cache.get() it will block
all consecutive calls until there is something to return from the cache.
Since there might be multiple threads calling different instances of the
Default class we need to prevent such thing from happening to avoid a
dead lock. What happens is that the first thread enters the method,
calls the hasElement() (which results in call to cache.get()), if the
key is not cached (the potential collision situation), the hasElement()
returns false and new CachePolicyResult (with request to store the key)
is created. If this thread finishes storing the key before next request
comes, everything is fine.
Let's assume the second request for same key comes _before_ the key is
stored, it will enter the synchronized block (because the first thread
already left it) and it will call the hasElement() (and consecutive call
to cache.get() will block the thread there in the synchronized block
until the cache key is stored and retrievable). So this second thread is
effectively stuck in the sync block, preventing anyone else from
entering it until the first thread is done with the retrieval of the
content for the key and caching the entry.
The point here is that we know exactly how many other threads might have
called the cache.get() method and can recover them by placing the value
later in the executor.Store.processCacheRequest() method (in the finally
block, notice we store the the cache key anyway, even if we can't
generate the entry to release the second thread that might be stuck in
the synchronized block in the Default cache policy). This works for as
long as there is max one thread stuck in the hasElement() (or more
precisely in cache.get()) call. If there will be more requests for the
key, they would be all released at a same time and they will all try to
generate the entry at a same time. This in itself should not be the
problem, but you need to keep in mind that entry creation is normally
pretty fast. If this situation occurred in the first place, it was
because either server is very busy, or producing response for this given
request is very heavy and slow. In either case you don't want to perform
it multiple times in parallel as it will not be faster then waiting for
the one processing thread to finish it and provide it as cached entry to
the others.

Hope this was clear enough.

More pointers - look at the svn history of the Default cache policy.
Look at the issues related to cache since Magnolia 3.6 (incl. the
milestone and RC issues).
Look at the EhCacheWrapper, Store executor and CacheFilter. Look at the
EhCache documentation and explanation of BlockingCache.


Cheers,
Jan


On Thu, 2009-10-08 at 15:55 +0100, Luke Biddell wrote:

>
> Hi folks,
>
> I've got some questions about the cache and hoped you wouldn't mind me
> harassing you about it. I've some queries around the cache
> implementation within Magnolia and was hoping you could set me
> straight.
>
>
> I've recently implemented a custom cache policy and as part of that
> work stumbled upon this piece of code within the
> info.magnolia.module.cache.cachepolicy.Default class.
>
>
> // we need to synchronize on the cache instance, as multiple threads
> might be accessing this
> // concurrently, and we don't want to block the system if we're using
> a blocking cache.
> // (since hasElement() might place a mutex on the cache key)
> synchronized (cache) {
>     if (cache.hasElement(key)) {
>         final Object cachedEntry = cache.get(key);
>         return new CachePolicyResult(CachePolicyResult.useCache, key,
> cachedEntry);
>     } else {
>         return new CachePolicyResult(CachePolicyResult.store, key,
> null);
>     }
> }
>
>
>
>
>
>
> 1) My first query is around the two calls to retrieve any given
> element. eg you first call hasElement and then get. Could we not just
> call get and check null as below?
>
>
>
>
>
>
> final Object cachedEntry = cache.get(key);
> if (cachedEntry != null) {
>       return new CachePolicyResult(CachePolicyResult.useCache, key,
> cachedEntry);
> } else {
>       return new CachePolicyResult(CachePolicyResult.store, key,
> null);
> }
>
>
>
> 2) The second query I have is about the use of the synchronised block
> around the cache retrieval. I've searched the entire codebase and all
> of the calls which add to the cache have no such synchronized block,
> only the retrieval does (I could easily have missed them though as I
> don't have the full source).
>
> The comment above the block doesn't make sense as by synchronising on
> the cache object you're only marshalling access to that section of
> code; subsequent calls to hasElement and get will block globally
> regardless of this?
>
> Under the covers, it appears to be using ehCache which is fully thread
> safe and no doubt uses appropriate read/write locking.
>
> I would also expect locking code to be encapsulated within the cache
> class implementation, the code which uses the cache can then be
> properly agnostic regarding the locking strategy.
>
> Thanks for any pointers you can give.
>
>
>
> Luke

----------------------------------------------------------------
For list details see
http://www.magnolia-cms.com/home/community/mailing-lists.html
To unsubscribe, E-mail to: <user-list-unsubscribe@...>
----------------------------------------------------------------

[MakkaPakka]

Thanks for the reply Jan. Took me several reads, think I understand now. I'll check the docco on ehcache.

2009/10/16 Jan Haderka <jan.haderka@...>

It's been a while, so bear with me.
I guess the comment should have read BlockingCache or even better the
net.st.ehcache.construct.blockingBlockingCache

As you have noticed Magnolia uses EhCache underneath. The EhCache
provides 2 implementations of EhCache interface, one of which is
blocking (in a sense that after first call to cache.get() it will block
all consecutive calls until there is something to return from the cache.
Since there might be multiple threads calling different instances of the
Default class we need to prevent such thing from happening to avoid a
dead lock. What happens is that the first thread enters the method,
calls the hasElement() (which results in call to cache.get()), if the
key is not cached (the potential collision situation), the hasElement()
returns false and new CachePolicyResult (with request to store the key)
is created. If this thread finishes storing the key before next request
comes, everything is fine.
Let's assume the second request for same key comes _before_ the key is
stored, it will enter the synchronized block (because the first thread
already left it) and it will call the hasElement() (and consecutive call
to cache.get() will block the thread there in the synchronized block
until the cache key is stored and retrievable). So this second thread is
effectively stuck in the sync block, preventing anyone else from
entering it until the first thread is done with the retrieval of the
content for the key and caching the entry.
The point here is that we know exactly how many other threads might have
called the cache.get() method and can recover them by placing the value
later in the executor.Store.processCacheRequest() method (in the finally
block, notice we store the the cache key anyway, even if we can't
generate the entry to release the second thread that might be stuck in
the synchronized block in the Default cache policy). This works for as
long as there is max one thread stuck in the hasElement() (or more
precisely in cache.get()) call. If there will be more requests for the
key, they would be all released at a same time and they will all try to
generate the entry at a same time. This in itself should not be the
problem, but you need to keep in mind that entry creation is normally
pretty fast. If this situation occurred in the first place, it was
because either server is very busy, or producing response for this given
request is very heavy and slow. In either case you don't want to perform
it multiple times in parallel as it will not be faster then waiting for
the one processing thread to finish it and provide it as cached entry to
the others.

Hope this was clear enough.

More pointers - look at the svn history of the Default cache policy.
Look at the issues related to cache since Magnolia 3.6 (incl. the
milestone and RC issues).
Look at the EhCacheWrapper, Store executor and CacheFilter. Look at the
EhCache documentation and explanation of BlockingCache.


Cheers,
Jan

On Thu, 2009-10-08 at 15:55 +0100, Luke Biddell wrote:
>
> Hi folks,
>
> I've got some questions about the cache and hoped you wouldn't mind me
> harassing you about it. I've some queries around the cache
> implementation within Magnolia and was hoping you could set me
> straight.
>
>
> I've recently implemented a custom cache policy and as part of that
> work stumbled upon this piece of code within the
> info.magnolia.module.cache.cachepolicy.Default class.
>
>
> // we need to synchronize on the cache instance, as multiple threads
> might be accessing this
> // concurrently, and we don't want to block the system if we're using
> a blocking cache.
> // (since hasElement() might place a mutex on the cache key)
> synchronized (cache) {
>     if (cache.hasElement(key)) {
>         final Object cachedEntry = cache.get(key);
>         return new CachePolicyResult(CachePolicyResult.useCache, key,
> cachedEntry);
>     } else {
>         return new CachePolicyResult(CachePolicyResult.store, key,
> null);
>     }
> }
>
>
>
>
>
>
> 1) My first query is around the two calls to retrieve any given
> element. eg you first call hasElement and then get. Could we not just
> call get and check null as below?
>
>
>
>
>
>
> final Object cachedEntry = cache.get(key);
> if (cachedEntry != null) {
>       return new CachePolicyResult(CachePolicyResult.useCache, key,
> cachedEntry);
> } else {
>       return new CachePolicyResult(CachePolicyResult.store, key,
> null);
> }
>
>
>
> 2) The second query I have is about the use of the synchronised block
> around the cache retrieval. I've searched the entire codebase and all
> of the calls which add to the cache have no such synchronized block,
> only the retrieval does (I could easily have missed them though as I
> don't have the full source).
>
> The comment above the block doesn't make sense as by synchronising on
> the cache object you're only marshalling access to that section of
> code; subsequent calls to hasElement and get will block globally
> regardless of this?
>
> Under the covers, it appears to be using ehCache which is fully thread
> safe and no doubt uses appropriate read/write locking.
>
> I would also expect locking code to be encapsulated within the cache
> class implementation, the code which uses the cache can then be
> properly agnostic regarding the locking strategy.
>
> Thanks for any pointers you can give.
>
>
>
> Luke

----------------------------------------------------------------
For list details see
http://www.magnolia-cms.com/home/community/mailing-lists.html
To unsubscribe, E-mail to: <user-list-unsubscribe@...>
----------------------------------------------------------------

[Jan Haderka-2]

You're welcome. I've been told by few other people that it was too dry
and condensed. I'll try to throw in few jokes next time I'm writing
something like this :D



On Fri, 2009-10-30 at 12:24 +0000, Luke Biddell wrote:

> Thanks for the reply Jan. Took me several reads, think I understand
> now. I'll check the docco on ehcache.
>
> 2009/10/16 Jan Haderka <jan.haderka@...>
>        
>         It's been a while, so bear with me.
>         I guess the comment should have read BlockingCache or even
>         better the
>         net.st.ehcache.construct.blockingBlockingCache
>        
>         As you have noticed Magnolia uses EhCache underneath. The
>         EhCache
>         provides 2 implementations of EhCache interface, one of which
>         is
>         blocking (in a sense that after first call to cache.get() it
>         will block
>         all consecutive calls until there is something to return from
>         the cache.
>         Since there might be multiple threads calling different
>         instances of the
>         Default class we need to prevent such thing from happening to
>         avoid a
>         dead lock. What happens is that the first thread enters the
>         method,
>         calls the hasElement() (which results in call to cache.get()),
>         if the
>         key is not cached (the potential collision situation), the
>         hasElement()
>         returns false and new CachePolicyResult (with request to store
>         the key)
>         is created. If this thread finishes storing the key before
>         next request
>         comes, everything is fine.
>         Let's assume the second request for same key comes _before_
>         the key is
>         stored, it will enter the synchronized block (because the
>         first thread
>         already left it) and it will call the hasElement() (and
>         consecutive call
>         to cache.get() will block the thread there in the synchronized
>         block
>         until the cache key is stored and retrievable). So this second
>         thread is
>         effectively stuck in the sync block, preventing anyone else
>         from
>         entering it until the first thread is done with the retrieval
>         of the
>         content for the key and caching the entry.
>         The point here is that we know exactly how many other threads
>         might have
>         called the cache.get() method and can recover them by placing
>         the value
>         later in the executor.Store.processCacheRequest() method (in
>         the finally
>         block, notice we store the the cache key anyway, even if we
>         can't
>         generate the entry to release the second thread that might be
>         stuck in
>         the synchronized block in the Default cache policy). This
>         works for as
>         long as there is max one thread stuck in the hasElement() (or
>         more
>         precisely in cache.get()) call. If there will be more requests
>         for the
>         key, they would be all released at a same time and they will
>         all try to
>         generate the entry at a same time. This in itself should not
>         be the
>         problem, but you need to keep in mind that entry creation is
>         normally
>         pretty fast. If this situation occurred in the first place, it
>         was
>         because either server is very busy, or producing response for
>         this given
>         request is very heavy and slow. In either case you don't want
>         to perform
>         it multiple times in parallel as it will not be faster then
>         waiting for
>         the one processing thread to finish it and provide it as
>         cached entry to
>         the others.
>        
>         Hope this was clear enough.
>        
>         More pointers - look at the svn history of the Default cache
>         policy.
>         Look at the issues related to cache since Magnolia 3.6 (incl.
>         the
>         milestone and RC issues).
>         Look at the EhCacheWrapper, Store executor and CacheFilter.
>         Look at the
>         EhCache documentation and explanation of BlockingCache.
>        
>        
>         Cheers,
>         Jan
>        
>        
>         On Thu, 2009-10-08 at 15:55 +0100, Luke Biddell wrote:
>         >
>         > Hi folks,
>         >
>         > I've got some questions about the cache and hoped you
>         wouldn't mind me
>         > harassing you about it. I've some queries around the cache
>         > implementation within Magnolia and was hoping you could set
>         me
>         > straight.
>         >
>         >
>         > I've recently implemented a custom cache policy and as part
>         of that
>         > work stumbled upon this piece of code within the
>         > info.magnolia.module.cache.cachepolicy.Default class.
>         >
>         >
>         > // we need to synchronize on the cache instance, as multiple
>         threads
>         > might be accessing this
>         > // concurrently, and we don't want to block the system if
>         we're using
>         > a blocking cache.
>         > // (since hasElement() might place a mutex on the cache key)
>         > synchronized (cache) {
>         >     if (cache.hasElement(key)) {
>         >         final Object cachedEntry = cache.get(key);
>         >         return new
>         CachePolicyResult(CachePolicyResult.useCache, key,
>         > cachedEntry);
>         >     } else {
>         >         return new
>         CachePolicyResult(CachePolicyResult.store, key,
>         > null);
>         >     }
>         > }
>         >
>         >
>         >
>         >
>         >
>         >
>         > 1) My first query is around the two calls to retrieve any
>         given
>         > element. eg you first call hasElement and then get. Could we
>         not just
>         > call get and check null as below?
>         >
>         >
>         >
>         >
>         >
>         >
>         > final Object cachedEntry = cache.get(key);
>         > if (cachedEntry != null) {
>         >       return new
>         CachePolicyResult(CachePolicyResult.useCache, key,
>         > cachedEntry);
>         > } else {
>         >       return new CachePolicyResult(CachePolicyResult.store,
>         key,
>         > null);
>         > }
>         >
>         >
>         >
>         > 2) The second query I have is about the use of the
>         synchronised block
>         > around the cache retrieval. I've searched the entire
>         codebase and all
>         > of the calls which add to the cache have no such
>         synchronized block,
>         > only the retrieval does (I could easily have missed them
>         though as I
>         > don't have the full source).
>         >
>         > The comment above the block doesn't make sense as by
>         synchronising on
>         > the cache object you're only marshalling access to that
>         section of
>         > code; subsequent calls to hasElement and get will block
>         globally
>         > regardless of this?
>         >
>         > Under the covers, it appears to be using ehCache which is
>         fully thread
>         > safe and no doubt uses appropriate read/write locking.
>         >
>         > I would also expect locking code to be encapsulated within
>         the cache
>         > class implementation, the code which uses the cache can then
>         be
>         > properly agnostic regarding the locking strategy.
>         >
>         > Thanks for any pointers you can give.
>         >
>         >
>         >
>         > Luke
>        
>        
>        
>         ----------------------------------------------------------------
>         For list details see
>         http://www.magnolia-cms.com/home/community/mailing-lists.html
>         To unsubscribe, E-mail to:
>         <user-list-unsubscribe@...>
>         ----------------------------------------------------------------
>        
>

----------------------------------------------------------------
For list details see
http://www.magnolia-cms.com/home/community/mailing-lists.html
To unsubscribe, E-mail to: <user-list-unsubscribe@...>
----------------------------------------------------------------