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

Use latest FSharpMap implementation #2182

Merged
merged 4 commits into from Sep 29, 2020
Merged

Conversation

alfonsogarciacaro
Copy link
Member

@alfonsogarciacaro alfonsogarciacaro commented Sep 27, 2020

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

@ncave
Copy link
Collaborator

ncave commented Sep 28, 2020

@alfonsogarciacaro Wrong target branch perhaps?

@alfonsogarciacaro alfonsogarciacaro changed the base branch from master to nagareyama-alpha September 28, 2020 01:40
@alfonsogarciacaro alfonsogarciacaro changed the base branch from nagareyama-alpha to nagareyama September 28, 2020 01:40
@alfonsogarciacaro
Copy link
Member Author

Ups, thanks!

@alfonsogarciacaro
Copy link
Member Author

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
After: 8595ms

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
After: 7997ms

@alfonsogarciacaro alfonsogarciacaro merged commit 946841d into nagareyama Sep 29, 2020
@ncave
Copy link
Collaborator

ncave commented Sep 30, 2020

@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.
Unfortunately micro-benchmarks can some times be misleading. Case in point is #2154, where the ResizeList is more than twice as fast than the LinkedList in micro-benchmarks (on both dotnet and nodejs), but for some reason it results in 10% slower perf on the FCS-JS benchmark.

@alfonsogarciacaro
Copy link
Member Author

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.

@ncave
Copy link
Collaborator

ncave commented Sep 30, 2020

@alfonsogarciacaro There's quite a few Reflection_typeTest in build/fable-library/Map.js and build/fable-library/Set.js, perhaps those can be optimized if they're slow.

@alfonsogarciacaro
Copy link
Member Author

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 instanceof, keeping or not keeping the warning as it can be quite verbose.

Atm Reflection_typeTest can only test generic parameters for arrays, lists and options, so it doesn't add a lot of value. We can remove and go back to inline type testing as before (without the error for generic types).

@ncave
Copy link
Collaborator

ncave commented Sep 30, 2020

We can remove and go back to inline type testing as before (without the error for generic types).

@alfonsogarciacaro Yes, perhaps we should revert and reitroduce the type testing change as a separate PR so we can test performance impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants