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

Fix some low-hanging fruit boxing in JsonSerializer#40867

Merged
stephentoub merged 1 commit intodotnet:masterfrom
stephentoub:jsonboxing
Sep 6, 2019
Merged

Fix some low-hanging fruit boxing in JsonSerializer#40867
stephentoub merged 1 commit intodotnet:masterfrom
stephentoub:jsonboxing

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Sep 6, 2019

It's boxing lots of dictionary enumerators and KeyValuePairs. Stop doing that.
cc: @steveharter, @ahsonkhan, @layomia

Before:
image

After:
image

Repro:

using System.IO;
using System.Text.Json;
using System.Threading.Tasks;

class Program
{
    static async Task Main()
    {
        var ms = new MemoryStream();
        var f = new Foo() { Value1 = 42, Value2 = 84.0 };
        for (int i = 0; i < 100_000; i++)
        {
            ms.Position = 0;
            await JsonSerializer.SerializeAsync(ms, f);
            ms.Position = 0;
            await JsonSerializer.DeserializeAsync<Foo>(ms);
        }
    }
}

public struct Foo
{
    public int Value1 { get; set; }
    public double Value2 { get; set; }
}

@stephentoub
Fix some low-hanging fruit boxing in JsonSerializer
aed65dc 
It's boxing lots of dictionary enumerators and KeyValuePairs.  Stop doing that.
@danmoseley
Copy link
Member

danmoseley commented Sep 6, 2019

Did a/could a analyzer help find these?

@scalablecory
Copy link

scalablecory commented Sep 6, 2019

Did a/could a analyzer help find these?

An analyzer to detect boxing sounds wonderful :).

@ahsonkhan ahsonkhan added this to the 5.0 milestone Sep 6, 2019
@ahsonkhan ahsonkhan added the tenet-performance Performance related issue label Sep 6, 2019
@stephentoub
Copy link
Member Author

stephentoub commented Sep 6, 2019

Did a analyzer help find these?

No. I was testing out the memory profiler and these jumped out at me (any time you see structs show up in the allocation list, it's a red flag).

could a analyzer help find these?

It could, but such analyzers are super, super noisy. There are lots of things in .NET that can result in hidden allocations, whether it be boxing, lambdas capturing locals, etc., and there are many legitimate uses for those.

An analyzer to detect boxing sounds wonderful :).

Such analyzers exist, e.g. https://github.com/Microsoft/RoslynClrHeapAllocationAnalyzer. But as noted above, they're much more useful in my experience as an exploratory tool, where you turn it on every once in a while locally and look for things you weren't expecting; as part of a build, they result in so much noise you quickly want to rip it out.

@stephentoub stephentoub merged commit 447f76f into dotnet:master Sep 6, 2019
@stephentoub stephentoub deleted the jsonboxing branch September 6, 2019 16:19
@ahsonkhan ahsonkhan mentioned this pull request Oct 14, 2019
Merged
@stephentoub stephentoub mentioned this pull request Feb 1, 2020
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@stephentoub
Fix some low-hanging fruit boxing in JsonSerializer (dotnet/corefx#40867
f27e64a 
)

It's boxing lots of dictionary enumerators and KeyValuePairs.  Stop doing that.

Commit migrated from dotnet/corefx@447f76f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@scalablecory scalablecory scalablecory left review comments

@ahsonkhan ahsonkhan ahsonkhan approved these changes

@steveharter steveharter steveharter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Text.Json tenet-performance Performance related issue

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

6 participants

@stephentoub @danmoseley @scalablecory @ahsonkhan @steveharter @Dotnet-GitSync-Bot