-
Notifications
You must be signed in to change notification settings - Fork 831
Experiment: Plug caches in #18468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Experiment: Plug caches in #18468
Changes from 5 commits
Commits
Show all changes
51 commits
Select commit
Hold shift + click to select a range
2e8000f
add caches
majocha 17f2d93
plug it in
majocha 17d2cbb
internal
majocha 3a0352c
Merge branch 'main' into plug-caches-in
majocha ced43c5
ok
majocha 08d3730
trace count in incremental use
majocha 7d10f96
Merge branch 'main' into plug-caches-in
majocha ffd764d
show count in release config too
majocha 1640e43
Merge branch 'plug-caches-in' of https://github.com/majocha/fsharp in…
majocha 5f2c535
tune cache
majocha a7d4605
fantomas
majocha 7d9746a
ilver
majocha 238a92a
fix sa again
majocha 1d21c78
just CWT for now
majocha 03dc52b
add some ids to see whats what
majocha d524663
for some reason it didnt work
majocha e130e01
metrics
majocha 47b4165
metrics
majocha 90eaa02
one cache instance per TcGlobals
majocha 165ea24
fix no eviction when OneOff
majocha 83208ac
restore LanguageFeature
majocha 4659934
singleton, but compilationMode aware
majocha 5abed61
fix background eviction
majocha a679a7f
Merge branch 'main' into plug-caches-in
majocha e1a48ff
wip
majocha e1cd30b
more metrics
majocha 7bdde6a
background eviction needs work
majocha ee872ca
Merge branch 'main' into plug-caches-in
majocha e6ba27e
fix stampEquals
majocha 5b87356
fix hash
majocha 039cc9a
improve allocations etc
majocha 0e8e6cb
format
majocha 474081a
ilverify
majocha c0ddd92
fix
majocha fdf1916
smaller cache
majocha 520c770
wip
majocha f01ac19
hit ratio
majocha 43cbdce
wip
majocha 8677d4f
wip
majocha 245460f
some cleanup
majocha 56eab5a
metrics
majocha b5f08cf
Merge branch 'main' into plug-caches-in
majocha 64b6107
add signature
majocha 905fe52
fix
majocha 6e21777
eviction
majocha 83d8122
output
majocha 2c81e17
back to singleton
majocha f9ab164
otel
majocha 6e6ca75
fix
majocha 95f6e56
Merge branch 'main' into plug-caches-in
majocha 619dbd4
fixfix
majocha File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,206 @@ | ||
| namespace FSharp.Compiler | ||
|
|
||
| open System | ||
| open System.Collections.Concurrent | ||
| open System.Threading | ||
| open System.Threading.Tasks | ||
| open System.Diagnostics | ||
|
|
||
| [<RequireQualifiedAccess>] | ||
| // Default Seq.* function have one issue - when doing `Seq.sortBy`, it will call a `ToArray` on the collection, | ||
| // which is *not* calling `ConcurrentDictionary.ToArray`, but uses a custom one instead (treating it as `ICollection`) | ||
| // this leads to and exception when trying to evict without locking (The index is equal to or greater than the length of the array, | ||
| // or the number of elements in the dictionary is greater than the available space from index to the end of the destination array.) | ||
| // this is casuedby insertions happened between reading the `Count` and doing the `CopyTo`. | ||
| // This solution introduces a custom `ConcurrentDictionary.sortBy` which will be calling a proper `CopyTo`, the one on the ConcurrentDictionary itself. | ||
| module internal ConcurrentDictionary = | ||
|
|
||
| open System.Collections | ||
| open System.Collections.Generic | ||
|
|
||
| let inline mkSeq f = | ||
| { new IEnumerable<'U> with | ||
| member _.GetEnumerator() = f () | ||
|
|
||
| interface IEnumerable with | ||
| member _.GetEnumerator() = (f () :> IEnumerator) | ||
| } | ||
|
|
||
| let inline mkDelayedSeq (f: unit -> IEnumerable<'T>) = mkSeq (fun () -> f().GetEnumerator()) | ||
|
|
||
| let inline sortBy ([<InlineIfLambda>] projection) (source: ConcurrentDictionary<_, _>) = | ||
| mkDelayedSeq (fun () -> | ||
| let array = source.ToArray() | ||
| Array.sortInPlaceBy projection array | ||
| array :> seq<_>) | ||
|
|
||
| [<Struct; RequireQualifiedAccess; NoComparison; NoEquality>] | ||
| type internal CachingStrategy = | ||
| | LRU | ||
| | LFU | ||
|
|
||
| [<Struct; RequireQualifiedAccess; NoComparison>] | ||
| type internal EvictionMethod = | ||
| | Blocking | ||
| | Background | ||
|
|
||
| [<Struct; RequireQualifiedAccess; NoComparison; NoEquality>] | ||
| type internal CacheOptions = | ||
| { | ||
| MaximumCapacity: int | ||
| PercentageToEvict: int | ||
| Strategy: CachingStrategy | ||
| EvictionMethod: EvictionMethod | ||
| LevelOfConcurrency: int | ||
| } | ||
|
|
||
| static member Default = | ||
| { | ||
| MaximumCapacity = 100 | ||
| PercentageToEvict = 5 | ||
| Strategy = CachingStrategy.LRU | ||
| LevelOfConcurrency = Environment.ProcessorCount | ||
| EvictionMethod = EvictionMethod.Blocking | ||
| } | ||
|
|
||
| [<Sealed; NoComparison; NoEquality>] | ||
| type internal CachedEntity<'Value> = | ||
| val Value: 'Value | ||
| val mutable LastAccessed: int64 | ||
| val mutable AccessCount: int64 | ||
|
|
||
| new(value: 'Value) = | ||
| { | ||
| Value = value | ||
| LastAccessed = DateTimeOffset.Now.Ticks | ||
| AccessCount = 0L | ||
| } | ||
|
|
||
| [<Sealed; NoComparison; NoEquality>] | ||
| [<DebuggerDisplay("{GetStats()}")>] | ||
| type internal Cache<'Key, 'Value> private (options: CacheOptions, capacity, cts) = | ||
|
|
||
| let cacheHit = Event<_ * _>() | ||
| let cacheMiss = Event<_>() | ||
| let eviction = Event<_>() | ||
|
|
||
| [<CLIEvent>] | ||
| member val CacheHit = cacheHit.Publish | ||
|
|
||
| [<CLIEvent>] | ||
| member val CacheMiss = cacheMiss.Publish | ||
|
|
||
| [<CLIEvent>] | ||
| member val Eviction = eviction.Publish | ||
|
|
||
| // Increase expected capacity by the percentage to evict, since we want to not resize the dictionary. | ||
| member val Store = ConcurrentDictionary<_, CachedEntity<'Value>>(options.LevelOfConcurrency, capacity) | ||
|
|
||
| static member Create(options: CacheOptions) = | ||
| let capacity = | ||
| options.MaximumCapacity | ||
| + (options.MaximumCapacity * options.PercentageToEvict / 100) | ||
|
|
||
| let cts = new CancellationTokenSource() | ||
| let cache = new Cache<'Key, 'Value>(options, capacity, cts) | ||
|
|
||
| if options.EvictionMethod = EvictionMethod.Background then | ||
| Task.Run(cache.TryEvictTask, cts.Token) |> ignore | ||
|
|
||
| cache | ||
|
|
||
| //member this.GetStats() = | ||
| // {| | ||
| // Capacity = options.MaximumCapacity | ||
| // PercentageToEvict = options.PercentageToEvict | ||
| // Strategy = options.Strategy | ||
| // LevelOfConcurrency = options.LevelOfConcurrency | ||
| // Count = this.Store.Count | ||
| // MostRecentlyAccesssed = this.Store.Values |> Seq.maxBy _.LastAccessed |> _.LastAccessed | ||
| // LeastRecentlyAccesssed = this.Store.Values |> Seq.minBy _.LastAccessed |> _.LastAccessed | ||
| // MostFrequentlyAccessed = this.Store.Values |> Seq.maxBy _.AccessCount |> _.AccessCount | ||
| // LeastFrequentlyAccessed = this.Store.Values |> Seq.minBy _.AccessCount |> _.AccessCount | ||
| // |} | ||
|
|
||
| member private this.CalculateEvictionCount() = | ||
| if this.Store.Count >= options.MaximumCapacity then | ||
| (this.Store.Count - options.MaximumCapacity) | ||
| + (options.MaximumCapacity * options.PercentageToEvict / 100) | ||
| else | ||
| 0 | ||
|
|
||
| // TODO: All of these are proofs of concept, a very naive implementation of eviction strategies, it will always walk the dictionary to find the items to evict, this is not efficient. | ||
| member private this.TryGetPickToEvict() = | ||
| this.Store | ||
| |> match options.Strategy with | ||
| | CachingStrategy.LRU -> ConcurrentDictionary.sortBy _.Value.LastAccessed | ||
| | CachingStrategy.LFU -> ConcurrentDictionary.sortBy _.Value.AccessCount | ||
| |> Seq.take (this.CalculateEvictionCount()) | ||
| |> Seq.map (fun x -> x.Key) | ||
|
|
||
| // TODO: Explore an eviction shortcut, some sort of list of keys to evict first, based on the strategy. | ||
| member private this.TryEvictItems() = | ||
| if this.CalculateEvictionCount() > 0 then | ||
| for key in this.TryGetPickToEvict() do | ||
| match this.Store.TryRemove(key) with | ||
| | true, _ -> eviction.Trigger(key) | ||
| | _ -> () // TODO: We probably want to count eviction misses as well? | ||
|
|
||
| // TODO: Shall this be a safer task, wrapping everything in try .. with, so it's not crashing silently? | ||
| member private this.TryEvictTask() = | ||
| backgroundTask { | ||
| while not cts.Token.IsCancellationRequested do | ||
| let evictionCount = this.CalculateEvictionCount() | ||
|
|
||
| if evictionCount > 0 then | ||
| this.TryEvictItems() | ||
|
|
||
| let utilization = (this.Store.Count / options.MaximumCapacity) | ||
| // So, based on utilization this will scale the delay between 0 and 1 seconds. | ||
| // Worst case scenario would be when 1 second delay happens, | ||
| // if the cache will grow rapidly (or in bursts), it will go beyond the maximum capacity. | ||
| // In this case underlying dictionary will resize, AND we will have to evict items, which will likely be slow. | ||
| // In this case, cache stats should be used to adjust MaximumCapacity and PercentageToEvict. | ||
| let delay = 1000 - (1000 * utilization) | ||
|
|
||
| if delay > 0 then | ||
| do! Task.Delay(delay) | ||
| } | ||
|
|
||
| member this.TryEvict() = | ||
| if this.CalculateEvictionCount() > 0 then | ||
| match options.EvictionMethod with | ||
| | EvictionMethod.Blocking -> this.TryEvictItems() | ||
| | EvictionMethod.Background -> () | ||
|
|
||
| member this.TryGet(key, value: outref<'Value>) = | ||
| match this.Store.TryGetValue(key) with | ||
| | true, cachedEntity -> | ||
| // this is fine to be non-atomic, I guess, we are okay with race if the time is within the time of multiple concurrent calls. | ||
| cachedEntity.LastAccessed <- DateTimeOffset.Now.Ticks | ||
| let _ = Interlocked.Increment(&cachedEntity.AccessCount) | ||
| cacheHit.Trigger(key, cachedEntity.Value) | ||
| value <- cachedEntity.Value | ||
| true | ||
| | _ -> | ||
| cacheMiss.Trigger(key) | ||
| value <- Unchecked.defaultof<'Value> | ||
| false | ||
|
|
||
| member this.TryAdd(key, value: 'Value, ?update: bool) = | ||
| let update = defaultArg update false | ||
|
|
||
| this.TryEvict() | ||
|
|
||
| let value = CachedEntity<'Value>(value) | ||
|
|
||
| if update then | ||
| let _ = this.Store.AddOrUpdate(key, value, (fun _ _ -> value)) | ||
| true | ||
| else | ||
| this.Store.TryAdd(key, value) | ||
|
|
||
| interface IDisposable with | ||
| member _.Dispose() = cts.Cancel() | ||
|
|
||
| member this.Dispose() = (this :> IDisposable).Dispose() | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there would be any drawbacks in implementing that
ToArrayspecial handling (via a typecheck) in theSeq.module directly.This is an unpleasant bug to deal with and I cannot thing of regressions when moving from old behavior to the one suggested here.
(big drawback is the added cost of type test for every single user, I do understand that :( )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be implemented and documented as "technically breaking, but correct change". I didn't bother with it, and just did the implementation I required in the cache itself.
Can we do it without type test via the inline compiler-only feature (i.e. type testing on type level and choosing correct implementation)?
This way, there's no runtime type test drawback.