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

Track JSON perf with source generation #33296

Merged
merged 7 commits into from
Jun 11, 2021
Merged

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Jun 4, 2021

No description provided.

@pranavkm pranavkm requested a review from a team as a code owner June 4, 2021 22:31
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jun 4, 2021
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Cool! What kind of numbers do we get with this?

@@ -55,12 +58,18 @@
void SerializeLarge()
=> serializedValue = JsonSerializer.Serialize(largeOrgChart);

void SerializeLargeSourceGen()
Copy link

Choose a reason for hiding this comment

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

Since these are benchmarks, is it worthwhile to also track the fastest perf here which would be:

using MemoryStream ms = new();
using Utf8JsonWriter writer = new(ms);
personJsonContext.Person.Serialize!(writer, largeOrgChart);
writer.Flush();
serializeValue = Encoding.UTF8.GetString(ms.ToArray());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, we'd like these to be good representation for what our users are doing. These benchmarks are much more useful for us to track relative performance improvements / regressions in these user scenarios rather than being absolute best-case numbers.

Copy link

Choose a reason for hiding this comment

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

The Serialize method is public based on the assumption that some users would want absolute-best perf and avoid some overhead of the serializer. Granted, some serializer features like max-depth handling would not be provided in this model, but do we not expect a non-trivial amount of users to want best-case perf?

@layomia
Copy link

layomia commented Jun 7, 2021

What kind of numbers do we get with this?

We won't see any perf improvement here yet since the serializer currently requires Strict number handling settings to allow fast-path logic to be used, while the benchmarks are specifying AllowReadingFromString via JsonSerializerDefaults.Web. This is not the correct check on the serializer side, since we really only need that WriteAsString is not specified. We'll need to fix this on the serializer side to see an improvement here - dotnet/runtime#53814.

Base automatically changed from prkrishn/use-source-generator to main June 7, 2021 19:19
@azure-pipelines azure-pipelines bot requested a review from Pilchie as a code owner June 7, 2021 19:19
Co-authored-by: Tanay Parikh <TanayParikh@users.noreply.github.com>
@pranavkm pranavkm merged commit a38936f into main Jun 11, 2021
@pranavkm pranavkm deleted the prkrishn/use-in-benchmarks branch June 11, 2021 17:58
@ghost ghost added this to the 6.0-preview6 milestone Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants