Introduce Sliding Cache to Constant Expression Helper

[TWhidden]

Introduce a Sliding Cache to ConstantExpressionHelper.

• Use of DateTime (UTC) instead of Stopwatch for expire period. lightweight
• Should still work with older frameworks - Still using same backing store, just with a private Tuple that holds the last accessed time
• Introduced new internal type ThreadSafeSlidingCache which has a constructor option of TTL for the object expire period, as well as an override for the cleanup frequency - default of 10 seconds atm - figured that would not add a huge strain, but can be adjusted as desired. did not know where we would put this class in the folder structure, feel free to refactor. It uses a "Lazy" Expire, so there is no timer, but objects expire when the cache is accessed.
• TTL default for objects built in Release is sliding cache value of 10 minutes. This is just an arbitrary number that seems reasonable.
• Test Case created - Passes on my end - feel free to refactor the name of the test

[StefH]

Thanks for this PR.

Can you look at the review comments please?

[TWhidden]

@StefH - Ok, implemented the feedback above, with the exception of a configuration process as I think that really isn't needed for this (not sure who would be trying to tune this, this is just the library keeping itself clean so the GC can do its job).

• Added a Unit Test for the ThreadSafeSlidingCache and all its functions.
• Removed the ConstantExpressionHelper since the unit tests for the ThreadSafeSlidingCache should be trusted.
• Dropped pre-processor directive I was using for tests, since implementing the Mock DateTime provider.

Thanks for the idea of the Mock DateTime - I can use that a lot on my own project tests!

[StefH]

Thanks for the idea of the Mock DateTime - I can use that a lot on my own project tests!
An official approach from Microsoft was is ISystemClock an an new one is https://learn.microsoft.com/en-us/dotnet/api/system.timeprovider?view=net-8.0, however that is only defined for .NET 8, so not usable in a lot of projects...

[TWhidden]

Feedback on the Latest Check-In:

1. Singleton Implementation in ConstantExpressionHelper: I've implemented an instance of ConstantExpressionHelper using a singleton pattern to prevent type duplication across instances. I'd appreciate any feedback on this pattern.

2. Testing - Setup Sequence: Seeking feedback on the setup sequence for tests. Setting up every datetime feels cumbersome, as the provider is extensively used within the Cache. Is it necessary to detail every access in the SetupSequence, or is setting the next time sufficient for these tests?

3. Issues with .NET Framework and Project Configurations:

   • Encountered issues with the .NET 4.0 targeting pack and .NET 4.5.2 compilation, unrelated to my changes. These issues stemmed from other types within the class.
   • I had to unload projects for .NET 3.5 (concerns about TaskUtil) and disable Android and UWP tests due to signing and certificate issues. My environment might require additional configuration for these tests.

4. New ParserConfig Options: Added three new ParserConfig options to reflect the different possibilities within the Cache of ConstantExpressionHelper, as suggested.

[StefH]

Thanks for the changes, only a few comments left. can you please take a look?

[StefH]

@TWhidden
Also, if you have solved my comment, you can "Resolve" this so that comment is closed in this issue page. Else I get lost reading all the open comments (which some are actually resolved) ;-)

Like:

[TWhidden]

My bad on "Resolve conversation" - that would have helped me also when going through them all.

Was thinking about the new CacheConfig - atm, it's just a POCO - should it be passed as an alternate constructor item to the ThreadSafeSlidingCache object? Just cleans up its usage a little bit, since the config is only for it.

Also - ThreadSafeSlidingCache just sounds silly - should we just call it SlidingCache ? Later, if there is another kind of cache, it wont have that same naming pattern?

[StefH]

Also - ThreadSafeSlidingCache just sounds silly - should we just call it SlidingCache ? Later, if there is another kind of cache, it wont have that same naming pattern?

SlidingCache is also fine for me.

[StefH]

Was thinking about the new CacheConfig - atm, it's just a POCO - should it be passed as an alternate constructor item to the ThreadSafeSlidingCache object? Just cleans up its usage a little bit, since the config is only for it.

If it makes it easier, yes you can do it.

[StefH]

@TWhidden
Thanks for the PR, I'll merge it and see if it correctly compiles on my system.

[StefH]

@TWhidden
Tomorrow I'll create a preview version which you can use to check if this indeed solves your issue.

[TWhidden]

Hey @StefH - We need to look a little into this. I think with high contention, we are creating some errors. Here is our Sentry report stack trace.

I propose some minor changes:
1.) in CleanupIfNeeded, use the Interlocked.CompareExchange, use this with a finally block as an atomic operation to keep possible multiple entries into Cleanup()
2.) _cache[key] I think is throwing this error; It may have been removed under contention, move to a TryGet
3.) In the TryGetValue of SlidingCache - instead of checking the expire time here, if it does exist, we should just update / return the value, Its a micro-optimization, but we are lazy removing anyways. if something is requesting it, maybe we should allow it to be returned, or make it a configuration to permit expired returns;

If you are ok with it, I would love to do another PR for it. Hopefully with less hassle this time to your process.

[TWhidden]

Ill Open a new issue - and submit a PR;