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.

Sign up for GitHub

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

Jump to bottom

perf: Change serializer for XrefMap from NewtonsoftJson to System.Text.Json #9832

Closed
wants to merge 10 commits into from
Closed

perf: Change serializer for XrefMap from NewtonsoftJson to System.Text.Json #9832

wants to merge 10 commits into from

Conversation

filzrev
Copy link
Contributor

@filzrev filzrev commented Apr 1, 2024 •
edited
Loading

This PR is successor of #9824.
Change XrefMap serializer from Newtonsoft.Json to System.Text.Json for perf optimization.

What's included in this PR

  1. Add SystemTextJsonUtility.cs and related converter for generic JSON serialization/deserialization.
  • This file is intended to be called from JsonUtility.cs wrapper class in future (By Migrating to System.Text.Json #9727).
    (And existing JsonUtility.cs logics will be moved to NewtonsoftJsonUtility.cs)
  1. Refactor XRefMapDownloader.cs
  • Change class to public sealed class.
  • Change to async-based API.
  • Change virtual methods that is not referenced from external to private.
  • Remove unused UpdateHref method.
  1. Fix wrong XRefMapRedirection.cs Href attribute that introduced by perf: Optimize xrefmap.json file deserialization performance #9824.

@yufeih yufeih added the performance Makes the pull request appear in "Performance" section of the next release note label Apr 2, 2024
@codecov Codecov
Copy link

codecov bot commented Apr 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 65.71429% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 78.98%. Comparing base (fe673ec) to head (1945061).
Report is 126 commits behind head on main.

❗ Current head 1945061 differs from pull request most recent head 583b309. Consider uploading reports for the commit 583b309 to get more accurate results

Files Patch % Lines
...System.Text.Json/ObjectToInferredTypesConverter.cs 34.61% 15 Missing and 2 partials ⚠️
...mon/Json/System.Text.Json/SystemTextJsonUtility.cs 81.81% 5 Missing and 1 partial ⚠️
src/Docfx.Build/XRefMaps/XRefMapDownloader.cs 90.90% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9832      +/-   ##
==========================================
+ Coverage   74.31%   78.98%   +4.66%     
==========================================
  Files         536      540       +4     
  Lines       23189    23372     +183     
  Branches     4056     4053       -3     
==========================================
+ Hits        17234    18461    +1227     
+ Misses       4853     3782    -1071     
- Partials     1102     1129      +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@filzrev filzrev mentioned this pull request Apr 12, 2024
Open
yufeih
yufeih reviewed Apr 12, 2024
View reviewed changes
src/Docfx.Common/Json/System.Text.Json/SystemTextJsonUtility.cs Outdated Show resolved Hide resolved
src/Docfx.Common/Json/System.Text.Json/SystemTextJsonUtility.cs
Converters =
{
new JsonStringEnumConverter(),
new ObjectToInferredTypesConverter(), // Required for `Dictionary<string, object>` type deserialization.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change Dictionary<string, object> Others -> Dictionary<string, JsonElement> Others if that is the only place using this converter?

filzrev and others added 5 commits April 14, 2024 16:53
@filzrev @yufeih
feat: Add docfx JSON Schema files and related tests (dotnet#9852)
526c94f 
chore: add json schema files and related tests

Co-authored-by: Yufei Huang <yufeih@live.com>
@dependabot @yufeih @filzrev
build(deps): bump Microsoft.Playwright from 1.42.0 to 1.43.0 (dotnet#…
cacd3b6 
…9860)

Bumps [Microsoft.Playwright](https://github.com/microsoft/playwright-dotnet) from 1.42.0 to 1.43.0.
- [Release notes](https://github.com/microsoft/playwright-dotnet/releases)
- [Commits](microsoft/playwright-dotnet@v1.42.0...v1.43.0)

---
updated-dependencies:
- dependency-name: Microsoft.Playwright
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Yufei Huang <yufeih@live.com>
@carlos-regis @filzrev
docs: Fix typo (dotnet#9871)
556450b
@filzrev
fix: Xrefmap baseUrl problem reported at dotnet#9866 (dotnet#9869)
7bc5eaa 
fix: xrefmap baseurl problems
@filzrev
chore: Add InternalVisibleTo attribute for benchmark project (dotne…
583b309 
…t#9840)

chore: add InternalVisibleTo attribute for benchmark project
@filzrev
Copy link
Contributor Author

filzrev commented Apr 14, 2024 •
edited
Loading

I've failed to rebase to latest main branch with resolve conflicts..
and other PR's commit are shown as diffs.

For clear diffs for code review. I'll recreate this PR later.
And fixing following remaining review comment.

Can we change Dictionary<string, object> Others -> Dictionary<string, JsonElement> Others if that is the only place using this converter?

@filzrev filzrev closed this Apr 14, 2024
@filzrev filzrev mentioned this pull request Apr 14, 2024
Merged
@filzrev filzrev deleted the perf-optimize-json-xrefmap-loading2 branch May 28, 2024 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yufeih yufeih yufeih left review comments

Assignees
No one assigned
Labels
performance Makes the pull request appear in "Performance" section of the next release note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@filzrev @yufeih @carlos-regis