Fix expensive typo in JsonNode #78130
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsFixes #78089 (but assuming this is the right fix, we should backport this to release/7.0)
|
|
Using the repro benchmark from the linked issue: using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Text.Json.Nodes;
[MemoryDiagnoser]
public partial class Program
{
static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
private JsonObject[] _jsonObjs = new JsonObject[100_000];
[Benchmark]
public void Sum()
{
for (int i = 0; i < 100_000; i++)
{
_jsonObjs[i] = new JsonObject()
{
["aaa"] = 1,
["bbbb"] = 2,
["cccc"] = new JsonArray()
{
new JsonObject()
{
["aaaa"] = "aaaaa",
["ffffff"] = "ffffffffffffff"
}
}
};
}
}
}
|
|
I assume this scenario is not covered in the dotnet/performance suite or we somehow missed this. |
|
Test failures are unrelated, mergin away. |
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3435456807 |
|
@stephentoub we have added the "Allocation ratio" column to BDN based on your request: dotnet/BenchmarkDotNet#722 and I can see that you have removed it here while I would expect this particular issue to be perfect use case for it. May I ask why? Does the implementation do not meet your requirements? |
Because in this case I was focusing on the throughput impact rather than on the allocation impact, and removed extra columns to try to focus attention (e.g. RatioSD, Gen0, and Alloc Ratio). The 6x improvement in throughput was the primary thing to show, rather than the 2x reduction in allocation. |
|
I can confirm that the newly added unit tests (dotnet/performance#2714) are shown as heavily regressing in the .NET 7 vs 6 GA performance report. Is there anything more that needs to be done to confirm we have addressed the problem with this backport, or are we confident this is a complete fix? System.Text.Json.Node.Tests.Perf_ParseThenWrite.ParseThenWrite(IsDataIndented: False, TestCase: LotsOfStrings)
System.Text.Json.Node.Tests.Perf_ParseThenWrite.ParseThenWrite(IsDataIndented: True, TestCase: LotsOfStrings)
System.Text.Json.Node.Tests.Perf_ParseThenWrite.ParseThenWrite(IsDataIndented: False, TestCase: Json400KB)
System.Text.Json.Node.Tests.Perf_ParseThenWrite.ParseThenWrite(IsDataIndented: False, TestCase: LotsOfNumbers)
(etc for all other benchmarks) |
|
Yes, we're fairly confident that the regression has been addressed. I've verified that the particular benchmark has parity between .NET 6 and the patched .NET 7 branch. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #78089 (but assuming this is the right fix, we should backport this to release/7.0)