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

Minor serialization tweaks #1170

Merged
merged 5 commits into from Sep 6, 2022
Merged

Minor serialization tweaks #1170

merged 5 commits into from Sep 6, 2022

Conversation

bkoelman
Copy link
Member

@bkoelman bkoelman commented Jul 18, 2022

The initial goal of this PR was to activate the System.Text.Json source generator. It is documented to improve performance and use less memory.

After making the change, I wasn't able to see improvements in benchmarks. Narrowing down the benchmark resulted in a performance gain of 2 microseconds and 838 bytes per request. Given all the trouble we've had in the past with source generators (bugs in the .NET SDK, increased build times, Resharper poorly supporting their use), I don't think it's worth making the change, so I reverted that commit. It's still in the PR commit history, for future reference.

This PR is still useful to merge because it also contains:

  • Remove unused parameters in a private method
  • Rename private Attribute class to match with .NET naming conventions
  • Make a private lazy member read-only and avoid potentially multiple initializations at startup
  • Use [JsonPropertyOrder], which enabled to simplify the JSON:API object model a bit
  • Tweaks in benchmarks project

QUALITY CHECKLIST

  • Changes implemented in code
  • Complies with our contributing guidelines
  • N/A: Adapted tests
  • N/A: Documentation updated

@bkoelman bkoelman force-pushed the json-sourcegen branch 4 times, most recently from d5ec214 to b48755f Compare July 18, 2022 22:05
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #1170 (f760e8f) into master (0185311) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1170      +/-   ##
==========================================
- Coverage   92.58%   92.57%   -0.01%     
==========================================
  Files         241      242       +1     
  Lines        7700     7694       -6     
==========================================
- Hits         7129     7123       -6     
  Misses        571      571              
Impacted Files Coverage Δ
...otNetCore/Serialization/Objects/AtomicReference.cs 100.00% <ø> (ø)
.../Serialization/Objects/ResourceIdentifierObject.cs 100.00% <ø> (ø)
...DotNetCore/Serialization/Objects/ResourceObject.cs 100.00% <ø> (ø)
...tNetCore/Serialization/Objects/SingleOrManyData.cs 100.00% <ø> (ø)
...Core/Serialization/Request/Adapters/BaseAdapter.cs 100.00% <ø> (ø)
...zation/Request/Adapters/ResourceIdentityAdapter.cs 100.00% <ø> (ø)
...n/Request/Adapters/ResourceIdentityRequirements.cs 100.00% <ø> (ø)
.../JsonApiDotNetCore/Configuration/JsonApiOptions.cs 100.00% <100.00%> (ø)
.../JsonApiDotNetCore/Middleware/JsonApiMiddleware.cs 97.77% <100.00%> (ø)
...tNetCore/Queries/Internal/Parsing/IncludeParser.cs 95.83% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bkoelman bkoelman force-pushed the json-sourcegen branch 3 times, most recently from f3bb2b0 to 2f23373 Compare August 21, 2022 12:04
…der (added in .NET 6). Also makes the model a bit more correct, as "ref" elements (used in atomic:operations) cannot occur in "data".
@bkoelman bkoelman changed the title Use System.Text.Json source generator Minor serialization tweaks Aug 21, 2022
@bkoelman bkoelman marked this pull request as ready for review August 21, 2022 14:52
@bkoelman bkoelman requested a review from maurei August 21, 2022 14:52
@maurei maurei merged commit 3069341 into master Sep 6, 2022
@maurei maurei deleted the json-sourcegen branch September 6, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants