Tagging subscribers to this area:@eerhardt,@maryamariyan
See info in area-owners.md if you want to be subscribed.

Now, most of the time related to caching (most of the time, in general, is spent in JSON serialization) and not related to the usage of Concurrent Dictionaryis spent in getting current time (to tell if the given entry has expired or not):

Have you tried making the current time check more efficient?

Have you tried making the current time check more efficient?

I did:#45281 Now it's just a syscall and I can't see a way to optimize it further

...because these should always be absolute stamps, in theory you could do something like get the current process/system time on cache initialization, via something likeEnvironment.TickCount64 or similar (obviously you'd need an abstraction for testing purposes, andTickCount64 is in milliseconds, which might not be sufficient), then use that for tracking expiration. This should remove the verification when getting the current time, although I don't know how much that would help.

@adamsitnik From having a quick look atMemoryCache there a few (small) quick wins left:

• Use .GetValueOrDefault() instead .Value (2x)

  runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

  Line 123 in14b3513

  if(!entry.AbsoluteExpiration.HasValue||absoluteExpiration.Value<entry.AbsoluteExpiration.Value)

• Change order of conditions (cheapest first)
  

  runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

  Line 139 in14b3513

  if(!entry.CheckExpired(utcNow)&&!exceedsCapacity)

• SinceICacheEntry.Size cannot benull whenMemoryCacheOptions.SizeLimit is set, you can useICacheEntry.Size.GetValueOrDefault() in quite a few places.
• Use .GetValueOrDefault() (instead of implicit .HasValue and .GetValueOrDefault())
  

  runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

  Line 111 in14b3513

  absoluteExpiration=utcNow+entry.AbsoluteExpirationRelativeToNow;

  
  

  runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

  Line 315 in14b3513

  if(newSize<0||newSize>_options.SizeLimit)

  
  

  runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

  Line 344 in14b3513

  double?lowWatermark=cache._options.SizeLimit*(1-cache._options.CompactionPercentage);

• Use for loop instead of foreach to iterate over List.

Don't hesitate to correct me!

@drieseng you are right, but the places that you have pointed are not on the hot path that I am optimizing. So even if I optimize them it won't have any effects

Embedding a timer in the implementation makes the component impossible to test reliably. Tests must be in full control of any timing. This is a blocking design point to resolve.

It's harder, but not impossible to mock.

The question is whether the testing ofMemoryCache should be exposed to the users? This would require me to introduce an official public abstraction forTimer similar to what you did withISystemClock.

This should remove the verification when getting the current time

I've already removed the redundant verification in#45281 WhateverEnvironment.TickCount64 uses, it's most probably a syscall and would not solve our problem

@drieseng you are right, but the places that you have pointed are not on the hot path that I am optimizing. So even if I optimize them it won't have any effects

It should affect theAddThenRemove andSetOverride benchmark results you've mentioned in the description... BUT... I'm saying this without having actually seen the benchmark code and without having tested the proposed changes :p

If you can guarantee that I can have the dotnet runtime built on my Windows box in 30 minutes, I definitely want to try to prove you wrong (or admit my defeat).

I may have missed some crucial implementation details, but could this work instead to achieve the objective?

• Change the signature ofCacheEntry.CheckExpired() to take aLazy<DateTimeOffset> instead of aDateTimeOffset. The implementation ofCacheEntry.CheckForExpiredTime() would only invoke the factory method (now would be changed tonow.Value) ifAbsoluteExpiration.HasValue istrue or_slidingExpiration.HasValue istrue.
• SetCacheEntry.LastAccessed only if_slidingExpiration.HasValue istrue.

Whatever Environment.TickCount64 uses, it's most probably a syscall

Not necessarily, Windows and some versions of Linux have a special memory page read-only mapped into all processes and this value is directly readable from it without a syscall.

It is also monolithically increasing unlike the real clock (which may be an attack vector for cache performance degradation).

I've already removed the redundant verification in#45281

... and, at least on windows, there's a whole bunch of stuff that happens when callingDateTime.UtcNow, like leap second handling.

at least on windows, there's a whole bunch of stuff that happens when calling DateTime.UtcNow, like leap second handling

I've optimized everything except leap seconds in the past there:dotnet/coreclr#26046

And now@GrabYourPitchforks is working on leap second handling perf:#44771

@adamsitnik
In a previouscomment, I did not take into accountCacheEntry.LastAccessed being updated on a lot of the public cache operations. You may want to have a look at a suggested direction for further optimization that I outlinedhere.

@edevoogd big thanks for your proposal! I really like the idea described inhttps://github.com/edevoogd/ClockQuantization!

For reasons described by@DamianEdwards onTwitter, I’ve decided to not pursue this idea. If we ever decide that we want to be in the Top 5 for this benchmark, we should reconsider it.

Yes we do want to do this.

I think you should still re-evaluate usingEnvironment.TickCount64 instead ofDateTime.UtcNow. The performance characteristics ought to be slightly different.

FWIW, local experimentation with changes inedevoogd/ClockQuantization#1 shows the additional benefit of usingEnvironment.TickCount64.

@filipnavara@adamsitnik