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 System.Reflection.Emit #15

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Use System.Reflection.Emit #15

wants to merge 8 commits into from

Conversation

Tarmil
Copy link
Owner

@Tarmil Tarmil commented Aug 10, 2019

Very WIP.

Currently only implemented for record serialization (struct records are failing). It's definitely an improvement over the previous results, especially in terms of runtime but also in terms of allocations (no more obj[]):

BenchmarkDotNet=v0.11.5, OS=Windows 10.0.18362
Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview7-012821
  [Host]   : .NET Core 3.0.0-preview7-27912-14 (CoreCLR 4.700.19.32702, CoreFX 4.700.19.36209), 64bit RyuJIT DEBUG
  ShortRun : .NET Core 3.0.0-preview7-27912-14 (CoreCLR 4.700.19.32702, CoreFX 4.700.19.36209), 64bit RyuJIT

Job=ShortRun  Runtime=Core  IterationCount=3
LaunchCount=1  WarmupCount=3

|                   Method | ArrayLength |        Mean |      Error |     StdDev |    Gen 0 |   Gen 1 |   Gen 2 | Allocated |
|------------------------- |------------ |------------:|-----------:|-----------:|---------:|--------:|--------:|----------:|
|     Serialize_Newtonsoft |          10 |    18.09 us |  11.246 us |  0.6164 us |   1.9836 |       - |       - |   8.13 KB |
| Serialize_SystemTextJson |          10 |    19.07 us |   4.051 us |  0.2221 us |   1.0071 |       - |       - |   4.13 KB |
|     Serialize_Newtonsoft |         100 |   170.73 us |  47.437 us |  2.6002 us |  19.7754 |  0.2441 |       - |   81.1 KB |
| Serialize_SystemTextJson |         100 |   188.28 us |  44.945 us |  2.4636 us |   9.5215 |  0.4883 |       - |  39.45 KB |
|     Serialize_Newtonsoft |        1000 | 1,781.06 us | 433.746 us | 23.7751 us | 119.1406 | 58.5938 | 58.5938 | 668.72 KB |
| Serialize_SystemTextJson |        1000 | 1,882.00 us | 338.485 us | 18.5535 us |  58.5938 | 58.5938 | 58.5938 | 392.77 KB |

@Tarmil
Copy link
Owner Author

Tarmil commented Aug 10, 2019

Got struct record serialization working, with similar results:

BenchmarkDotNet=v0.11.5, OS=Windows 10.0.18362
Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview7-012821
  [Host]   : .NET Core 3.0.0-preview7-27912-14 (CoreCLR 4.700.19.32702, CoreFX 4.700.19.36209), 64bit RyuJIT DEBUG
  ShortRun : .NET Core 3.0.0-preview7-27912-14 (CoreCLR 4.700.19.32702, CoreFX 4.700.19.36209), 64bit RyuJIT

Job=ShortRun  Runtime=Core  IterationCount=3
LaunchCount=1  WarmupCount=3

|                   Method | ArrayLength |        Mean |      Error |     StdDev |    Gen 0 |    Gen 1 |   Gen 2 | Allocated |
|------------------------- |------------ |------------:|-----------:|-----------:|---------:|---------:|--------:|----------:|
|     Serialize_Newtonsoft |          10 |    20.23 us |   7.511 us |  0.4117 us |   2.2278 |        - |       - |   9.16 KB |
| Serialize_SystemTextJson |          10 |    21.09 us |   5.055 us |  0.2771 us |   1.6479 |        - |       - |    6.8 KB |
|     Serialize_Newtonsoft |         100 |   198.45 us |  55.239 us |  3.0278 us |  22.2168 |   0.4883 |       - |  91.45 KB |
| Serialize_SystemTextJson |         100 |   208.40 us |  35.732 us |  1.9586 us |  16.1133 |        - |       - |  66.21 KB |
|     Serialize_Newtonsoft |        1000 | 1,945.98 us |  28.079 us |  1.5391 us | 175.7813 | 113.2813 | 58.5938 | 787.93 KB |
| Serialize_SystemTextJson |        1000 | 2,138.60 us | 241.417 us | 13.2329 us | 117.1875 |  58.5938 | 58.5938 | 660.35 KB |

@baronfel
Copy link
Contributor

Just wanted to say that this PR is very instructional and I'm very hopeful for it! With Npgsql recently releasing support for System.Text.Json for postgresql json/jsonb columns it'd be wonderful to have a more 'safe' serializer for types coming directly from the database.

@Tarmil Tarmil force-pushed the master branch 2 times, most recently from 16a58f0 to 0822481 Compare September 30, 2019 11:59
@baronfel
Copy link
Contributor

@Tarmil Thought: is it worth packaging out the reflection-emit + type cache into a separate package? It would be nice to use them in other projects as well and share the same instances.

@Tarmil
Copy link
Owner Author

Tarmil commented Oct 30, 2019

Yeah, I've considered publishing this as an emit-based non-allocating alternative to FSharp.Reflection. I need to at least implement it for unions though, and it's harder than records 🙂

@baronfel
Copy link
Contributor

There's an interesting PR on the dotnet/fsharp repo that might sidestep this PR: dotnet/fsharp#9714

If that gets in, then this repo could just use the new method to generate a mostly-optimized reader func. Still not quite as complete/fast as your implementation here I believe, though.

@Tarmil
Copy link
Owner Author

Tarmil commented Jul 18, 2020

This looks like a nice improvement, thanks for the heads up @baronfel !

@baronfel
Copy link
Contributor

Aaaaand take two is coming already: https://github.com/dotnet/fsharp/pull/9784/files

@shanoaice
Copy link

Will this be an optional feature if it is implemented? Since the NativeAOT mode of .NET 7 does not support System.Reflection.Emit and my humble opinion is that AOT usage cases are worth considering

@Tarmil
Copy link
Owner Author

Tarmil commented Feb 12, 2023

As the author of Bolero, which runs on WebAssembly, I fully agree that environments where Emit is unavailable must and will always remain supported!

That being said, as you can see this is quite an old and partial branch, and I really don't know if and when I'll resume work on it anyway.

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.

3 participants