Skip to content
This repository was archived by the owner on Aug 2, 2023. It is now read-only.

[WIP] Optimize JsonWriterUtf8 WriteValue overloads#2332

Closed
cshung wants to merge 2 commits into
dotnet:masterfrom
cshung:dev/andrewau/2313
Closed

[WIP] Optimize JsonWriterUtf8 WriteValue overloads#2332
cshung wants to merge 2 commits into
dotnet:masterfrom
cshung:dev/andrewau/2313

Conversation

@cshung
Copy link
Copy Markdown
Contributor

@cshung cshung commented Jun 3, 2018

Does this pattern sounds like a way to go?

@cshung cshung requested a review from ahsonkhan June 3, 2018 02:07
@ahsonkhan ahsonkhan added area-System.Text.Json Hackathon Issues picked for Hackathon on 2018/6/2 labels Jun 3, 2018
@ahsonkhan
Copy link
Copy Markdown
Contributor

ahsonkhan commented Jun 3, 2018 •
edited
Loading

Does this pattern sounds like a way to go?

I am not sure. I think we should compare the performance of refactoring the code this way.

Update this performance benchmark to write bool values instead of long values and see how performance is affected:

corefxlab/tests/Benchmarks/System.Text.JsonLab/JsonWriterPerf.cs

Line 125 in da50fe3

json.WriteValue(data[i]);

Also add the DisassemblyDiagnoser attribute to the benchmark test to see the assembly that gets generated. It will help us see if this approach is worth it.
[DisassemblyDiagnoser(printAsm: true, printPrologAndEpilog: true, recursiveDepth: 0)]

If you need to see how to run the benchmarks, see https://github.com/dotnet/corefxlab#measuring-performance

ahsonkhan
ahsonkhan reviewed Jun 3, 2018
View reviewed changes
Comment thread src/System.Text.JsonLab/System/Text/Json/JsonWriterUtf8.cs
WriteJsonValue(JsonConstants.FalseValue);
int seperatorSize = ItemSeperatorSize();
int spacingSize = SpacingSize();
int literalSize = LiteralSize(literal);
Copy link
Copy Markdown
Contributor

@ahsonkhan ahsonkhan Jun 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unnecessary. We should just use literal.Length where its needed.

ahsonkhan
ahsonkhan reviewed Jun 3, 2018
View reviewed changes
Comment thread src/System.Text.JsonLab/System/Text/Json/JsonWriterUtf8.cs
Span<byte> byteBuffer = EnsureBuffer(bytesNeeded);

int idx = 0;
idx += this.WriteItemSeperator(byteBuffer.Slice(idx, seperatorSize));
Copy link
Copy Markdown
Contributor

@ahsonkhan ahsonkhan Jun 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do all these in a single method to avoid the multiple span slices and additions to idx?

ahsonkhan
ahsonkhan reviewed Jun 3, 2018
View reviewed changes
Comment thread src/System.Text.JsonLab/System/Text/Json/JsonWriterUtf8.cs
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private int WriteSpacing(Span<byte> byteBuffer)
Copy link
Copy Markdown
Contributor

@ahsonkhan ahsonkhan Jun 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the return values of these methods always equal what the "{X}Size()" methods return?

WriteSpacing would return whatever SpacingSize does. So, why do we need to return an integer here?

@cshung
Copy link
Copy Markdown
Contributor Author

cshung commented Jun 3, 2018

Thanks for the review and the pointer to measuring performance. I am experimenting with performance now.

@cshung
Copy link
Copy Markdown
Contributor Author

cshung commented Jun 3, 2018 

Before change:

                         Method | IsUTF8Encoded | Formatted |         Mean |      Error |     StdDev |  Gen 0 | Allocated |
------------------------------- |-------------- |---------- |-------------:|-----------:|-----------:|-------:|----------:|
      WriterSystemTextJsonBasic |         False |     False | 28,952.25 ns | 35.2831 ns | 33.0038 ns |      - |       0 B |
          WriterNewtonsoftBasic |         False |     False | 12,655.46 ns | 26.3697 ns | 24.6662 ns | 0.2899 |     472 B |
 WriterSystemTextJsonHelloWorld |         False |     False |     79.88 ns |  0.3013 ns |  0.2671 ns |      - |       0 B |
     WriterNewtonsoftHelloWorld |         False |     False |    241.86 ns |  0.1887 ns |  0.1576 ns | 0.1979 |     312 B |
      WriterSystemTextJsonBasic |         False |      True | 35,835.25 ns | 77.7731 ns | 68.9438 ns |      - |       0 B |
          WriterNewtonsoftBasic |         False |      True | 25,667.60 ns | 61.7226 ns | 54.7155 ns | 0.3967 |     640 B |
 WriterSystemTextJsonHelloWorld |         False |      True |     85.45 ns |  0.2113 ns |  0.1873 ns |      - |       0 B |
     WriterNewtonsoftHelloWorld |         False |      True |    353.39 ns |  0.4913 ns |  0.4596 ns | 0.3047 |     480 B |
      WriterSystemTextJsonBasic |          True |     False | 27,262.53 ns | 69.0936 ns | 61.2496 ns |      - |       0 B | <-
          WriterNewtonsoftBasic |          True |     False | 14,797.07 ns | 34.7243 ns | 32.4811 ns | 0.2594 |     416 B |
 WriterSystemTextJsonHelloWorld |          True |     False |    140.36 ns |  0.1685 ns |  0.1494 ns |      - |       0 B |
     WriterNewtonsoftHelloWorld |          True |     False |    291.21 ns |  0.5943 ns |  0.5268 ns | 0.1626 |     256 B |
      WriterSystemTextJsonBasic |          True |      True | 33,533.96 ns | 43.7357 ns | 38.7705 ns |      - |       0 B | <-
          WriterNewtonsoftBasic |          True |      True | 26,819.86 ns | 81.6822 ns | 72.4091 ns | 0.3662 |     584 B |
 WriterSystemTextJsonHelloWorld |          True |      True |    146.10 ns |  0.7797 ns |  0.7293 ns |      - |       0 B |
     WriterNewtonsoftHelloWorld |          True |      True |    393.75 ns |  7.7484 ns |  7.6099 ns | 0.2694 |     424 B |

After change:

                         Method | IsUTF8Encoded | Formatted |         Mean |       Error |      StdDev |  Gen 0 | Allocated |
------------------------------- |-------------- |---------- |-------------:|------------:|------------:|-------:|----------:|
      WriterSystemTextJsonBasic |         False |     False | 29,005.02 ns |  61.4929 ns |  54.5119 ns |      - |       0 B |
          WriterNewtonsoftBasic |         False |     False | 12,797.02 ns |  21.6237 ns |  19.1688 ns | 0.2899 |     472 B |
 WriterSystemTextJsonHelloWorld |         False |     False |     79.97 ns |   0.1896 ns |   0.1681 ns |      - |       0 B |
     WriterNewtonsoftHelloWorld |         False |     False |    248.35 ns |   0.7189 ns |   0.6724 ns | 0.1979 |     312 B |
      WriterSystemTextJsonBasic |         False |      True | 35,835.13 ns |  74.1917 ns |  69.3990 ns |      - |       0 B |
          WriterNewtonsoftBasic |         False |      True | 25,436.63 ns |  52.1356 ns |  46.2169 ns | 0.3967 |     640 B |
 WriterSystemTextJsonHelloWorld |         False |      True |     88.03 ns |   0.1957 ns |   0.1831 ns |      - |       0 B |
     WriterNewtonsoftHelloWorld |         False |      True |    396.02 ns |   1.0286 ns |   0.9118 ns | 0.3047 |     480 B |
      WriterSystemTextJsonBasic |          True |     False | 17,008.23 ns | 347.7084 ns | 427.0171 ns |      - |       0 B | <-
          WriterNewtonsoftBasic |          True |     False | 14,822.00 ns |  41.2945 ns |  36.6065 ns | 0.2594 |     416 B |
 WriterSystemTextJsonHelloWorld |          True |     False |    133.46 ns |   0.1781 ns |   0.1488 ns |      - |       0 B |
     WriterNewtonsoftHelloWorld |          True |     False |    291.66 ns |   0.5487 ns |   0.5133 ns | 0.1626 |     256 B |
      WriterSystemTextJsonBasic |          True |      True | 18,200.41 ns |  51.7895 ns |  45.9100 ns |      - |       0 B | <-
          WriterNewtonsoftBasic |          True |      True | 26,805.09 ns |  47.6850 ns |  42.2715 ns | 0.3662 |     584 B |
 WriterSystemTextJsonHelloWorld |          True |      True |    141.58 ns |   0.2967 ns |   0.2776 ns |      - |       0 B |
     WriterNewtonsoftHelloWorld |          True |      True |    386.46 ns |   0.8276 ns |   0.7741 ns | 0.2694 |     424 B |

@cshung cshung force-pushed the dev/andrewau/2313 branch from 090eec8 to 934925f Compare June 3, 2018 20:40
@ahsonkhan
Copy link
Copy Markdown
Contributor

ahsonkhan commented Jun 3, 2018

WriterSystemTextJsonBasic | True | False | 17,008.23 ns |
WriterNewtonsoftBasic | True | False | 14,822.00 ns |

What changes did you make to get those results? It seems to be an out-lier. All other UTF-8 benchmarks show SystemTextJson is faster. We should try to optimize the implementation further for this case as well.

@ahsonkhan ahsonkhan mentioned this pull request Jun 6, 2018
Merged
@cshung cshung closed this Jun 25, 2018
@cshung cshung deleted the dev/andrewau/2313 branch June 25, 2018 20:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ahsonkhan ahsonkhan ahsonkhan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@cshung cshung

Labels

area-System.Text.Json Hackathon Issues picked for Hackathon on 2018/6/2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cshung @ahsonkhan