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

[WIP] Stop using reflection to deserialize JSON #2403

Closed
wants to merge 13 commits into from
Closed

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Feb 10, 2021

One of the possible solutions to #2343.

JSON.NET methods like JObject.SerializeObject, .DeserializeObject, .ToObject, .Value use reflection. Updates code to serialize/deserialize objects by calling methods that don't use reflection, like JObject[] array indexers.
(Also merges JsonUtils class into JsonHelpers.)

  • Create ISerializable interface with Serialize/Deserialize methods and implement in classes that are dependent on OAuth2Client (MsalTokenResponse, InstanceDiscoveryResponse, InstanceDiscoveryMetadataEntry, AdfsWebFingerResponse, DeviceCodeResponse).
  • Update unit tests to pass with new code.
  • Write unit tests to compare that new serialization/deserialization is the same as old.
  • Update other needed classes so that silent & device code flows work end-to-end (ClientInfo, LocalImdsResponse)
  • Run perf tests for serializing with new and old code.

Perf test results
.NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X86 RyuJIT

Method Categories Mean Error StdDev Ratio
Serialize, MsalTokenResponse, With Reflection Serialize, MsalTokenResponse 4.920 μs 0.0548 μs 0.0513 μs 1.00
Serialize, MsalTokenResponse, Without Reflection Serialize, MsalTokenResponse 8.210 μs 0.0867 μs 0.0768 μs 1.67
Deserialize, MsalTokenResponse, With Reflection Deserialize, MsalTokenResponse 9.363 μs 0.1209 μs 0.0944 μs 1.00
Deserialize, MsalTokenResponse, Without Reflection Deserialize, MsalTokenResponse 10.654 μs 0.0851 μs 0.0710 μs 1.14
Serialize, InstanceDiscoveryResponse, With Reflection Serialize, InstanceDiscoveryResponse 41.122 μs 0.4426 μs 0.3923 μs 1.00
Serialize, InstanceDiscoveryResponse, Without Reflection Serialize, InstanceDiscoveryResponse 51.787 μs 0.9862 μs 1.0962 μs 1.25
Deserialize, InstanceDiscoveryResponse, With Reflection Deserialize, InstanceDiscoveryResponse 70.212 μs 0.6181 μs 0.5782 μs 1.00
Deserialize, InstanceDiscoveryResponse, Without Reflection Deserialize, InstanceDiscoveryResponse 65.974 μs 1.1809 μs 1.1046 μs 0.94
Serialize, OAuth2ResponseBase, With Reflection Serialize, OAuth2ResponseBase 2.137 μs 0.0356 μs 0.0297 μs 1.00
Serialize, OAuth2ResponseBase, Without Reflection Serialize, OAuth2ResponseBase 2.804 μs 0.0068 μs 0.0061 μs 1.31
Deserialize, OAuth2ResponseBase, With Reflection Deserialize, OAuth2ResponseBase 3.861 μs 0.0767 μs 0.0753 μs 1.00
Deserialize, OAuth2ResponseBase, Without Reflection Deserialize, OAuth2ResponseBase 3.847 μs 0.0285 μs 0.0267 μs 1.00

@bgavrilMS
Copy link
Member

Looks good so far

@bgavrilMS
Copy link
Member

Great stuff.

@pmaytak pmaytak changed the title Stop using reflection to deserialize JSON [WIP] Stop using reflection to deserialize JSON Feb 24, 2021
@pmaytak
Copy link
Contributor Author

pmaytak commented Mar 2, 2021

Went with #2428 as a solution as it's less invasive and more reliable.

@pmaytak pmaytak closed this Mar 2, 2021
@pmaytak pmaytak deleted the pmaytak/json branch March 2, 2021 07:06
@pmaytak pmaytak restored the pmaytak/json branch March 2, 2021 07:07
@pmaytak pmaytak deleted the pmaytak/json branch March 2, 2021 20:23
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