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

Implement reflection cache for unions, tuples, options and maps #254

Merged
merged 1 commit into from
Jun 12, 2021

Conversation

bklop
Copy link
Contributor

@bklop bklop commented Jun 11, 2021

Hi Zaid! I recently identified a performance issue when serializing one of our F# domain models. The performance issue was caused by many repeated reflection calls at runtime in the FableConverter.

The "worst case" seems to be large DUs as keys in a Map, with many items in the Map. Our model looks something like this:

type MyUnion =
    | Case1
    | Case2
    ...
    | Case200

type MyRecord {
    Map1: Map<MyUnion, string>
    Map2: Map<MyUnion, string>
    ...
    Map20: Map<MyUnion, string> }

I've made some performance improvements which basically boil down to:

  • Caching reflection calls for unions (including options) and tuples
  • Building lookup tables for union case names (mainly for deserialization)
  • Pre-computing constructors for unions and tuples
  • Caching map serialization instances to avoid runtime MethodInfo.Invoke calls

For our domain model (basically the code structure mentioned above) we saw serialization time decrease from 450ms to 1.5ms, which we're pretty happy with!

@Zaid-Ajaj
Copy link
Owner

This is really cool @bklop 🔥 the code looks good too!

Will try to merge and publish soon 😄

@bklop
Copy link
Contributor Author

bklop commented Jun 11, 2021

Thanks! But take your time to review it for correctness 😄. I tested it on a proprietary application + I made sure all tests pass (obviously).

@Zaid-Ajaj
Copy link
Owner

I made a benchmark project just for this, check it out here

The performance is insane 🔥 🔥 🔥

I made a large union (500 cases) and a large record (200 fields of type Map<LargeUnion, string>) and tested the JSON handling against the current JSON library and your implementation:

$ dotnet run
Running operations 10 time(s)
Old (Serialization): 2335L ms on average
New (Serialization): 425L ms on average
Old (Deserialization): 87517L ms on average
New (Deserialization): 4L ms on average
Old (Full convertion): 81919L ms on average
New (Full convertion): 2L ms on average

@Zaid-Ajaj Zaid-Ajaj merged commit 9c219dc into Zaid-Ajaj:master Jun 12, 2021
Zaid-Ajaj added a commit that referenced this pull request Jun 12, 2021
@Zaid-Ajaj
Copy link
Owner

PR merged and has been published into Fable.Remoting.Json v2.17 and all downstream packages that depend on it. Thanks a lot @bklop for the awesome contribution ❤️

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.

2 participants