Skip to content
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

Cache delegates #14582

Closed
cmeeren opened this issue Jan 10, 2023 · 7 comments
Closed

Cache delegates #14582

cmeeren opened this issue Jan 10, 2023 · 7 comments

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Jan 10, 2023

Consider this code:

open System.Collections.Concurrent

let d = ConcurrentDictionary<string, int>()

let getOrAdd key = d.GetOrAdd(key, fun _ -> 0)

for _ = 0 to 1_000_000 do
  getOrAdd "" |> ignore

When memory profiling (using dotMemory, targeting .NET 7), I see that this allocates 1 million Func<String, Int32> objects:

Allocated type : System.Func<String, Int32>
  Objects : 1000001
  Bytes   : 64000064

Allocated by
   100%  getOrAdd • 61,04 MB / 61,04 MB • global::Program.getOrAdd(String)
     100%  main@ • 61,04 MB / - • <StartupCode$AllocTest>.$Program.main@()
      ►  100%  [AllThreadsRoot] • 61,04 MB / - • [AllThreadsRoot]

This is very surprising to me. (Although I'm no expert in esoteric subjects like compiler optimizations, I consider myself a fairly experienced F# developer.)

I propose that F# caches these delegates. (Note: I am unsure if "delegates" is the right term.)

Possibly related: #910, dotnet/runtime#80430

@T-Gro
Copy link
Member

T-Gro commented Jan 11, 2023

I think this would be the way to reduce the allocation:

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AbEAzAzmgExAGoAfCABxgDsACAZQE9cAXGAWwDoBhCDDGGBYBLCNVw8xYAK5RY1FgFgAUCoEtaBWgF5avajLk0WAEWFDR1AIZRGAHlZRh1AOZpazlgD4AFAEoVNRgNMCswAAsYAgAxaQMdBmY2LliDOwB9NHTfbDjadNo4L1oABgDVZXVaF2CAeSgAQQItAGsYRgSCTgBxOsbmnzbGd1CIqNSwcpVsaHyEktoWCFoARnSSjfWNzQgVWmq+pq0AImPaUmLhF2poGCA=

I wonder if there is any other situation besides Action<> and Func<,> family where similar thing is needed, or if this optimization would be targeting them only.

@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 11, 2023

Might be a good idea to look into caching functions when we can.

it's quite big language/compiler feature, please, create a suggestion over at https://github.com/fsharp/fslang-suggestions/issues (or maybe look if there's existing one).

Here are some semi-related issuess for reference:
fsharp/fslang-suggestions#1131
fsharp/fslang-suggestions#1083

@vzarytovskii vzarytovskii closed this as not planned Won't fix, can't repro, duplicate, stale Jan 11, 2023
@github-project-automation github-project-automation bot moved this from Not Planned to Done in F# Compiler and Tooling Jan 11, 2023
@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 11, 2023

I wonder if there is any other situation besides Action<> and Func<,> family where similar thing is needed, or if this optimization would be targeting them only.

@T-Gro I think it's mainly Func<> and Action<>, but still might be a good thing to have (ASP.NET APIs come to mind, though I think that JIT can optimize it for us :D)

@kerams
Copy link
Contributor

kerams commented Jan 11, 2023

fsharp/fslang-suggestions#1083

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 12, 2023

C# always cache delegates created in this way

I'm not clear on the details/differences, but note that there are delegates or similar that C# does not cache, as demonstrated in dotnet/runtime#80430.

@jl0pd
Copy link

jl0pd commented Jan 12, 2023

I'm not clear on the details/differences

Essentially Foo(x => Bar(x)) was cached, but Foo(Bar) wasn't

brianrourkeboll added a commit to brianrourkeboll/fsharp that referenced this issue May 9, 2024
* Override `ToString` on `BuildPhase`.

* Cache the delegate passed into `ConcurrentDictionary.GetOrAdd` where
  possible. See dotnet#14582, fsharp/fslang-suggestions#1083, etc.
psfinaki added a commit that referenced this issue May 13, 2024
* Minor compiler perf improvements

* Override `ToString` on `BuildPhase`.

* Cache the delegate passed into `ConcurrentDictionary.GetOrAdd` where
  possible. See #14582, fsharp/fslang-suggestions#1083, etc.

* Name

* Update release notes

* Fmt

* Remove unneeded `StringBuilder`

* Start count at 1

* Go back to `GetOrAdd`

* I don't think I fully understand why, but I did some rough
  microbenchmarking, and for some reason the `GetOrAdd` and
  `Interlocked.Increment` on a ref cell technique is actually something
  like twice as fast as `AddOrUpdate`.

---------

Co-authored-by: Petr <psfinaki@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

5 participants