-
Notifications
You must be signed in to change notification settings - Fork 301
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
Use latest FSharpMap implementation #2182
Conversation
@alfonsogarciacaro Wrong target branch perhaps? |
Ups, thanks! |
bb95dc2
to
4c962b9
Compare
4c962b9
to
1026666
Compare
Some simple tests: measureTime <| fun () ->
let map = Seq.init 1000000 (fun i -> Guid.NewGuid(), i) |> Map
for _ = 1 to 1000000 do
let g2 = Guid.NewGuid()
let _ = Map.containsKey g2 map
() Before: 12932ms measureTime <| fun () ->
let set = Seq.init 1000000 (fun _ -> Guid.NewGuid()) |> set
for _ = 1 to 1000000 do
let g2 = Guid.NewGuid()
let _ = Set.contains g2 set
() Before: 13122ms |
@alfonsogarciacaro Unfortunately this PR results in 5% slower performance in the FCS-JS benchmark. Please note I'm not saying it needs to be rolled back, or even that the FCS-JS benchmark is what we should be using to measure performance, just stating the fact. I haven't looked at perf reports to see where the loss is coming from, cause most of the time it's quite hard to pinpoint it. |
Interesting, thanks for checking this 👍 I have no problem in reverting the change, it looks indeed like some changes that look good in microbenchmark can cause deoptimizations in bigger apps. That said, I made the mistake of adding some cleanup for Array.fs in this PR, I hope it was not related. Can you please check #2187 (reverts Map/Set but not Array) to see if it fixes the performance regression? It'd be nice to have another computation intensive app to test besides FCS as a tie-breaker in these situations. Hopefully after announcing Nagareyama some users step up as it happened with Fable 2. |
@alfonsogarciacaro There's quite a few |
Ah, you're right! These were part of my (unsuccessful) effort to support generic parameters in type testing #2092, we should likely compile them as Atm |
@alfonsogarciacaro Yes, perhaps we should revert and reitroduce the type testing change as a separate PR so we can test performance impact. |
EDIT: Now this also includes the latest FSharpSet implementation and a bit of cleanup for the Array module. Simple benchmark here.
Sorry @ncave, got ahead of you with this ;) Seems the latest FSharpMap implementation is much faster so I tried using it. See dotnet/fsharp#10188
There are some tests failing, I will fix them.
There's another PR open for FSharpSet. We could use it too or wait until the PR is merged dotnet/fsharp#10190