Skip to content

Add opt-in for converters to handle null#35826

Merged
layomia merged 11 commits intodotnet:masterfrom
layomia:converter_handle_null
May 8, 2020
Merged

Add opt-in for converters to handle null#35826
layomia merged 11 commits intodotnet:masterfrom
layomia:converter_handle_null

Conversation

@layomia
Copy link
Contributor

@layomia layomia commented May 5, 2020

Fixes #34439.

@layomia layomia added this to the 5.0 milestone May 5, 2020
@layomia layomia requested review from jozkee and steveharter May 5, 2020 04:31
@layomia layomia requested a review from ahsonkhan as a code owner May 5, 2020 04:31
@layomia layomia self-assigned this May 5, 2020
@ghost
Copy link

ghost commented May 5, 2020

Tagging subscribers to this area: @jozkee
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Dotnet-GitSync-Bot commented May 5, 2020

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@layomia
Copy link
Contributor Author

layomia commented May 5, 2020

This change does not regress existing benchmarks.

dotnet run --base "D:\benchmarks\handle_null\before" --diff "D:\benchmarks\handle_null\after" --threshold 3%
summary:
better: 1, geomean: 1.061
worse: 2, geomean: 1.036
total diff: 3
Slower diff/base Base Median (ns) Diff Median (ns) Modality
System.Text.Json.Serialization.Tests.WriteJson.Serial 1.04 692.62 717.90
System.Text.Json.Serialization.Tests.WriteJson.Seria 1.04 349.66 362.06
Faster base/diff Base Median (ns) Diff Median (ns) Modality
System.Text.Json.Serialization.Tests.WriteJson.SerializeToString 1.06 410.06 386.40

before.zip
after.zip

@jaredpar jaredpar mentioned this pull request May 7, 2020
Closed
@layomia layomia mentioned this pull request May 8, 2020
Merged
@layomia layomia merged commit 7949ab8 into dotnet:master May 8, 2020
@layomia layomia deleted the converter_handle_null branch May 8, 2020 17:56
jozkee
jozkee reviewed May 8, 2020
View reviewed changes
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@jozkee jozkee jozkee left review comments

@ahsonkhan ahsonkhan Awaiting requested review from ahsonkhan

+1 more reviewer

@steveharter steveharter steveharter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@layomia layomia

Labels

area-System.Text.Json new-api-needs-documentation

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

Provide opt-in for custom converters to handle null

5 participants

@layomia @Dotnet-GitSync-Bot @stephentoub @steveharter @jozkee