A bottlenecked error in Nop Cache ?

Hi there.

I'm researching the Nop Core. I think there are a bottlenecked problem in the Nop Cache, it is  Nop.Core.Caching.CacheExtensions.Get<T>.

This is an extension method which extends the function of ICacheManager, and the problem is "lock (_syncObject)" command:
            lock (_syncObject)
            {
                if (cacheManager.IsSet(key))
                {
                    return cacheManager.Get<T>(key);
                }

                var result = acquire();
                if (cacheTime > 0)
                    cacheManager.Set(key, result, cacheTime);
                return result;
            }
It use _syncObject, a static object to obtain the mutual-exclusion lock :
private static readonly object _syncObject = new object();

The issue is Get<T>() extension method is used for both PerRequestCacheManager and MemoryCacheManager, and it use only a single object, _syncObject, to hold lock.

Suppose we have one MemoryCacheManager instance ( yep, it's singleton ), and 10 PerRequestCacheManager instances at the same time, and all of them want to call Get<T>() method, only one of them can obtain the _syncObject lock and execute, and the remains have to wait.

Understanding that MemoryCacheManager instance should be thread safe, however PerRequestCacheManager instances would be proceeded asynchronously.
Is it an error ?

Hi,

It's by design. In your scenario all 10 PerRequestCacheManager instances have the same implementation. Hence they cache in the same Cache instance (memory). All the rest should wait.

P.S. PerRequestCacheManager is not instanciated only once (SingleInstance) - please have a look at \Presentation\Nop.Web.Framework\DependencyRegistrar.cs

Well, PerRequestCache relies on HttpContext.Current.Items ( or FakeHttpContext.Item in background thread/no httpcontext.current context ), so can i say it's "singeton per request" ?

I add a command to Application_BeginRequest and write two simple action methods in HomeController:
        protected void Application_BeginRequest(object sender, EventArgs e)
        {
            Context.Items["Now__"] = DateTime.UtcNow;

        
         public ActionResult Test1()
        {
            var cache = EngineContext.Current.ContainerManager.Resolve<ICacheManager>("nop_cache_per_request");
            var name = cache.Get<string>("Key1", () => {
                Thread.Sleep(4000);
                return "Test1";
            });
            string message =
                string.Format("Hello, I'm {0}. I spent {1} miliseconds to get my name. My cache type is {2}",
                    name, (DateTime.UtcNow - (DateTime)HttpContext.Items["Now__"]).TotalMilliseconds, cache.GetType().FullName);
            return Content(message);
        }

        public ActionResult Test2()
        {
            var cache = EngineContext.Current.ContainerManager.Resolve<ICacheManager>("nop_cache_per_request");
            var name = cache.Get<string>("Key2", () => {
                return "Test2";
            });

            string message =
                string.Format("Hello, I'm {0}. I spent {1} miliseconds to get my name. My cache type is {2}",
                    name, (DateTime.UtcNow - (DateTime)HttpContext.Items["Now__"]).TotalMilliseconds, cache.GetType().FullName);
            return Content(message);
        }


It's action methods that test the behavious of "nop_cache_per_request" ICacheManager, or PerRequestCache. I open firefox and chrome, enter two url http://localhost:15522/Home/Test2 and http://localhost:15522/Home/Test1, and request them concurrently, Home/Test2 first and Home/Test1 a little late. This is my results:
Hello, I'm Test2. I spent 10.0072 miliseconds to get my name. My cache type is Nop.Core.Caching.PerRequestCacheManager
Hello, I'm Test1. I spent 4013.1256 miliseconds to get my name. My cache type is Nop.Core.Caching.PerRequestCacheManager
If you noticed, Test2 responsed almost immediate, and Test1 responsed after 4 seconds.


Now, I reverse the order of them, request Test1 first and Test2 after. And the results:
Hello, I'm Test1. I spent 4028.8526 miliseconds to get my name. My cache type is Nop.Core.Caching.PerRequestCacheManager
Hello, I'm Test2. I spent 3781.8185 miliseconds to get my name. My cache type is Nop.Core.Caching.PerRequestCacheManager
Well, Test2 must wait until Test1 release the lock, so it must wait 4 seconds ! But the job that Test1 do don't need to hold lock, it only play with it's HttpContext.Items collection, and this can be done independently. What happen when we have 1000 user, one user requets /Test1 and the rest request /Test2 ? 999 user must wait one user when he do his individual job ?
That's not my expectation in this context !


The issue is you have one MemoryCacheManager instance (static memory cache, singleton) and many many PerRequestCacheManager instance ( one instance per request ), but you use only one lock object for all of them. Each PerRequestCacheManager instance has an independent memory, they have very short-lived life, and they don't need a read/write lock . If you must give them a lock object, you should give one new object for each PerRequestCacheManager instance.



Microsoft always say about asynchronous programming, they allow us make requests asynchronous and recommence us to do that. Each request has an independent HttpContext, can and must run asynchronously, but now you lock and force them execute step by step ???

sounj142 wrote:



+5000
I totally agree with you.

I am caching 60.000 product list in Application cache with no lock, async 1000 request for 60.000 return result in less than 3 secs.

This is problematic NOP issue I think.

Hi,

I've just analyzed the code one more time. And you're absolutely right! Thanks a lot for suggestion and good example! Fixed

a.m. wrote:

+++ :)
Thanks Andre

a.m. wrote:
Thanks andrei, but you say

ignificant performance optimization of caching implementation (for high-loaded sites). We should not use "lock" in CachingExtensions ("get" method).

You mean we should use lock ? is that a mistake ?

Everything is correct. We should not use "lock". And we don't use it anymore