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

Update JsonWriter performance tests and add more unit tests.#2293

Merged
ahsonkhan merged 6 commits into
dotnet:masterfrom
ahsonkhan:UpdateJsonWriterTests
May 17, 2018
Merged

Update JsonWriter performance tests and add more unit tests.#2293
ahsonkhan merged 6 commits into
dotnet:masterfrom
ahsonkhan:UpdateJsonWriterTests

Conversation

@ahsonkhan
Copy link
Copy Markdown
Contributor

@ahsonkhan ahsonkhan commented May 16, 2018

[WIP] Depends on #2292 (where we split the types) for CI to be green.

BenchmarkDotNet=v0.10.14.534-nightly, OS=Windows 10.0.16299.431 (1709/FallCreatorsUpdate/Redstone3)
Intel Xeon CPU E5-1620 v2 3.70GHz, 1 CPU, 8 logical and 4 physical cores
Frequency=3604598 Hz, Resolution=277.4234 ns, Timer=TSC
.NET Core SDK=2.1.300-rtm-008838
  [Host]     : .NET Core 2.1.0-rtm-26514-02 (CoreCLR 4.6.26514.02, CoreFX 4.6.26514.02), 64bit RyuJIT
  DefaultJob : .NET Core 2.1.0-rtm-26514-02 (CoreCLR 4.6.26514.02, CoreFX 4.6.26514.02), 64bit RyuJIT

Method IsUTF8Encoded Formatted Mean Error StdDev Median Gen 0 Allocated
WriterSystemTextJsonBasic False False 13,007.23 ns 206.9702 ns 193.6001 ns 13,007.52 ns - 0 B
WriterNewtonsoftBasic False False 24,385.41 ns 323.5019 ns 302.6039 ns 24,460.31 ns 0.0610 472 B
WriterSystemTextJsonHelloWorld False False 79.65 ns 0.1798 ns 0.1501 ns 79.68 ns - 0 B
WriterNewtonsoftHelloWorld False False 252.18 ns 2.8943 ns 2.2597 ns 251.23 ns 0.0591 312 B
WriterSystemTextJsonBasic False True 15,455.75 ns 27.5224 ns 22.9825 ns 15,455.74 ns - 0 B
WriterNewtonsoftBasic False True 38,726.82 ns 423.6548 ns 353.7709 ns 38,878.64 ns 0.0610 640 B
WriterSystemTextJsonHelloWorld False True 85.28 ns 1.5050 ns 1.4078 ns 86.04 ns - 0 B
WriterNewtonsoftHelloWorld False True 351.49 ns 6.7202 ns 6.9011 ns 356.92 ns 0.0911 480 B
WriterSystemTextJsonBasic True False 13,229.05 ns 18.3385 ns 14.3175 ns 13,228.26 ns - 0 B
WriterNewtonsoftBasic True False 24,424.51 ns 237.3358 ns 210.3920 ns 24,457.86 ns 0.0610 416 B
WriterSystemTextJsonHelloWorld True False 141.67 ns 2.8499 ns 2.7990 ns 143.30 ns - 0 B
WriterNewtonsoftHelloWorld True False 286.15 ns 4.9989 ns 4.6760 ns 284.86 ns 0.0486 256 B
WriterSystemTextJsonBasic True True 15,354.43 ns 178.5350 ns 158.2666 ns 15,431.49 ns - 0 B
WriterNewtonsoftBasic True True 37,463.68 ns 97.7627 ns 86.6641 ns 37,465.13 ns 0.0610 584 B
WriterSystemTextJsonHelloWorld True True 145.69 ns 0.3185 ns 0.2659 ns 145.73 ns - 0 B
WriterNewtonsoftHelloWorld True True 384.19 ns 6.8438 ns 6.4017 ns 387.05 ns 0.0806 424 B

Copy link
Copy Markdown
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM!

I saw the that the unit tests are called "basic" and you don't check for edge case support.

I assume that you plan to do this in the future, so please don't forget to check some edge cases (like a string containing ",", nested json etc). I wonder how handling such edge cases is going to affect the performance.

_stringBuilder.Clear();
writer = new StringWriter(_stringBuilder);
}
WriterNewtonsoftHelloWorld(Formatted, writer);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you please move the duplicated code to a helper methd?

TextWriter writer;
if (IsUTF8Encoded)
{
	_memoryStream.Seek(0, SeekOrigin.Begin);
	writer = _streamWriter;
}
else
{
	_stringBuilder.Clear();
	writer = new StringWriter(_stringBuilder);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, will do.

_symbolTable = GetTargetEncoder(Target);
_arrayFormatter = new ArrayFormatter(BufferSize, _symbolTable);
_data = new int[ExtraArraySize];
Random rand = new Random(42);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good idea to create Random with constant seed! 👍

@ahsonkhan ahsonkhan changed the title [WIP] Update JsonWriter performance tests and add more unit tests. Update JsonWriter performance tests and add more unit tests. May 16, 2018
@ahsonkhan
Copy link
Copy Markdown
Contributor Author

@dotnet-bot test this please

1 similar comment
@ahsonkhan
Copy link
Copy Markdown
Contributor Author

@dotnet-bot test this please

@ahsonkhan
Copy link
Copy Markdown
Contributor Author

@dotnet-bot test Innerloop Ubuntu16.04 Debug Build and Test

@ahsonkhan ahsonkhan merged commit cae7771 into dotnet:master May 17, 2018
@ahsonkhan ahsonkhan deleted the UpdateJsonWriterTests branch May 17, 2018 03:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants