Skip to content

Conversation

@svick
Copy link
Contributor

@svick svick commented Oct 13, 2019

Related to dotnet/corefx#41753 and https://github.com/dotnet/corefx/issues/41638.

This is a case where System.Text.Json currently has a performance issue, so I think it makes sense to add a benchmark for it, independently of whether the PR dotnet/corefx#41753 is accepted into corefx.

Copy link
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, thank you!

Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Thanks!

@ahsonkhan
Copy link
Contributor

With your fix from https://github.com/dotnet/corefx/issues/41638, is the performance of SerializeToString and SerializeObjectProperty almost identical now or is there still some gap here?

@adamsitnik
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@billwert
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik adamsitnik merged commit 20d7acd into dotnet:master Oct 15, 2019
@svick
Copy link
Contributor Author

svick commented Oct 15, 2019

@ahsonkhan There is still some gap, though considering that SerializeObjectProperty does more, I'd say it's fairly small. For example, for the Dictionary<string, string> case, SerializeToString takes ~44 us, while SerializeObjectProperty takes now ~51 us (for comparison, previously it was over 2200 us).

But I haven't looked further into it, so it's possible there are still other optimizations to be had.

@svick svick deleted the json-serialize-object-property branch October 15, 2019 19:08
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.

4 participants