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

Add benchmarks for ISet<T> (de)serialization and general framework fo… #540

Merged
merged 3 commits into from
Jul 18, 2019

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Jun 4, 2019

…r generic collections

cc @ahsonkhan, @steveharter, @adamsitnik , @billwert


namespace System.Text.Json.Serialization.Tests
{
public class WriteGenericCollection
Copy link
Member

Choose a reason for hiding this comment

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

File mismatch. ReadJson.X has the test for Write and vice versa.

Copy link
Member

Choose a reason for hiding this comment

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

Is there another commit coming that resolves this @layomia ?

}

[Benchmark]
public ISet<string> DeserializeISet()
Copy link
Member

@ahsonkhan ahsonkhan Jun 5, 2019

Choose a reason for hiding this comment

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

Please share the perf results of any new tests added within the PR you add them in.

Nevermind, I see it in the PR: dotnet/corefx#38210

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the Error/Std Dev of some of the results shared, I recommend commenting out the RecommendedConfig before running the tests locally to improve accuracy.

For example, this is quite inaccurate:

DeserializeISet 2 21,030.9 ns 3,649.67 ns 10,412.72 ns

.WithWarmupCount(1) // 1 warmup is enough for our purpose
.WithIterationTime(TimeInterval.FromMilliseconds(250)) // the default is 0.5s per iteration, which is slighlty too much for us
.WithMinIterationCount(15)
.WithMaxIterationCount(20) // we don't want to run more that 20 iterations

@adamsitnik
Copy link
Member

@layomia similar to other stale PRs (#478, #530, #589) I have cloned your fork, updated the code and pushed some fixes. I need to make sure that we don't regress it for 3.0. PTAL at my changes

dotnet run -c Release -f netcoreapp3.0 --filter *WriteJson<*Set*>* *ReadJson<*Set*>* --join
BenchmarkDotNet=v0.11.3.1003-nightly, OS=Windows 10.0.18362
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
.NET Core SDK=3.0.100-preview7-012697
  [Host]     : .NET Core 3.0.0-preview7-27826-20 (CoreCLR 4.700.19.32603, CoreFX 4.700.19.32613), 64bit RyuJIT
  Job-KNHVRD : .NET Core 3.0.0-preview7-27826-20 (CoreCLR 4.700.19.32603, CoreFX 4.700.19.32613), 64bit RyuJIT

IterationTime=250.0000 ms  MaxIterationCount=20  MinIterationCount=15  
WarmupCount=1  
Type Method Mean Error StdDev Median Min Max Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
ReadJson<HashSet<String>> DeserializeFromString 25.18 us 0.6991 us 0.8051 us 24.92 us 24.32 us 27.16 us 2.5324 0.1013 - 20632 B
WriteJson<HashSet<String>> SerializeToString 15.46 us 0.1955 us 0.1829 us 15.55 us 15.16 us 15.69 us 0.7500 - - 6128 B
ReadJson<HashSet<String>> DeserializeFromUtf8Bytes 24.31 us 1.1398 us 1.2669 us 24.03 us 23.07 us 27.25 us 2.1780 0.0947 - 17648 B
WriteJson<HashSet<String>> SerializeToUtf8Bytes 15.14 us 0.2402 us 0.2129 us 15.09 us 14.82 us 15.59 us 0.3669 - - 3168 B
ReadJson<HashSet<String>> DeserializeFromStream 24.27 us 0.4657 us 0.5177 us 24.19 us 23.43 us 25.03 us 2.2014 0.0957 - 17720 B
WriteJson<HashSet<String>> SerializeToStream 14.83 us 0.1804 us 0.1688 us 14.81 us 14.63 us 15.12 us - - - 184 B

@layomia
Copy link
Contributor Author

layomia commented Jul 18, 2019

@adamsitnik LGTM, thanks!

@layomia layomia deleted the iset branch July 18, 2019 18:53
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

4 participants