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

Add .NET 6 target that uses System.Text.Json and IL trimming #3605

Merged
merged 42 commits into from
Sep 2, 2022
Merged

Add .NET 6 target that uses System.Text.Json and IL trimming #3605

merged 42 commits into from
Sep 2, 2022

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Aug 16, 2022 •
edited
Loading

Fixes #1550, fixes #3407

Changes proposed in this request

  • Added NET6 target to MSAL project.
  • Used System.Text.Json (STJ) package in NET 6 target.
  • Enabled IL trimming for NET 6 target.
  • Merged JsonUtils.cs into JsonHelper.cs.
  • Used type aliases to force Newtonsoft types to actually refer to equivalent STJ types. This way existing JSON code stays the same, if API is the same between two libraries.
  • For some code that has different API, moved it into JsonHelper, where the preprocessor flags are applied, so it's all in one place.
  • Tried to make as little refactoring as possible to limit the risk of regression.
  • Added NET 6 target to unit, integration, perf tests.
  • In NET 6 unit tests, excluded WAM related tests from running.
  • Dismiss warning that NetCore2.1 is obsolete.
  • Dismiss iOS tests because they can't build .NET 6 target.

Example of an app that calls AcquireTokenInteractive and was built as self-contained:

Target Trimmed Size
netcoreapp3.1 no 35 MB
net6.0 no 58 MB
net6.0 yes 13 MB

Testing

  • Updated unit and integration tests to run on NET 6.
  • Tested IL trimming manually with a dev app.
  • Didn't notice any build time increase.

Performance impact
The rows represent different tests benchmarked. The columns represent the comparison -

  • previous code using Newtonsoft (tested on netcoreapp3.1 target),
  • refactored code in this PR using Newtonsoft on netcoreapp3.1,
  • new code in this PR using System.Text.Json on net6.0.

The before and after code that uses Newtonsoft should be the same, included here to check for regressions.

Test results for current PR:

Method CacheSize EnableCacheSerialization Mean
(previous Net3.1)		 Mean
(new Net3.1)		 Mean
(new Net6)		 Allocated
(previous Net3.1)		 Allocated
(new Net3.1)		 Allocated
(new Net6)
Serialize_MsalTokenResponse_Test - - 6.597 us 6.798 us 1.479 us 4 KB 4 KB 2 KB
Deserialize_MsalTokenResponse_Test - - 12.476 us 12.807 us 3.167 us 5 KB 5 KB 1 KB
Serialize_InstanceDiscoveryResponse_Test - - 41.827 us 42.486 us 13.003 us 27 KB 27 KB 10 KB
Deserialize_InstanceDiscoveryResponse_Test - - 75.548 us 73.417 us 27.090 us 23 KB 23 KB 15 KB
Serialize_OAuth2ResponseBase_Test - - 2.240 us 2.239 us 1.746 us 2 KB 2 KB 1 KB
Deserialize_OAuth2ResponseBase_Test - - 4.200 us 4.339 us 1.323 us 4 KB 4 KB 1 KB
AcquireTokenForClient - - 268.615 us 278.705 us 265.245 us 70 KB 70 KB 56 KB
AcquireTokenOBO - - 365.387 us 367.687 us 342.475 us 131 KB 131 KB 100 KB
AcquireTokenForClient (1, 10) False 16.291 us 16.482 us 12.216 us 22 KB 22 KB 20 KB
AcquireTokenForOBO (1, 10) False 29.822 us 29.368 us 22.854 us 35 KB 35 KB 29 KB
AcquireTokenForClient (1, 10) True 170.303 us 171.427 us 107.139 us 271 KB 271 KB 217 KB
AcquireTokenForOBO (1, 10) True 254.616 us 230.642 us 144.363 us 326 KB 326 KB 300 KB
AcquireTokenSilent (1, 10) - 22.583 us 21.981 us 16.864 us 28 KB 28 KB 26 KB
GetAccount (1, 10) - 7.373 us 7.528 us 4.745 us 7 KB 7 KB 7 KB
RemoveAccount (1, 10) - 36.115 us 36.908 us 33.763 us 26 KB 26 KB 26 KB
AcquireTokenForClient (1, 1000) False 221.821 us 222.968 us 181.582 us 131 KB 131 KB 131 KB
AcquireTokenForOBO (1, 1000) False 265.578 us 270.902 us 217.105 us 144 KB 144 KB 144 KB
AcquireTokenForClient (1, 1000) True 25,523.188 us 26,093.618 us 13,025.475 us 18,351 KB 18,359 KB 19,438 KB
AcquireTokenForOBO (1, 1000) True 30,656.492 us 17,322.703 us 20,047.856 us 21,255 KB 21,253 KB 23,004 KB
AcquireTokenSilent (1, 1000) - 257.373 us 249.261 us 205.166 us 137 KB 137 KB 137 KB
GetAccount (1, 1000) - 7.069 us 7.568 us 4.952 us 7 KB 7 KB 7 KB
RemoveAccount (1, 1000) - 952.846 us 959.247 us 821.534 us 1,790 KB 1,790 KB 1,831 KB
AcquireTokenForClient (10000, 10) False 34.047 us 34.505 us 28.496 us 22 KB 22 KB 20 KB
AcquireTokenForOBO (10000, 10) False 46.471 us 46.846 us 37.620 us 35 KB 35 KB 29 KB
AcquireTokenForClient (10000, 10) True 175.618 us 166.482 us 108.532 us 271 KB 271 KB 217 KB
AcquireTokenForOBO (10000, 10) True 591.638 us 498.314 us 148.095 us 326 KB 331 KB 300 KB
AcquireTokenSilent (10000, 10) - 39.891 us 39.807 us 33.507 us 28 KB 28 KB 26 KB
GetAccount (10000, 10) - 23.944 us 23.566 us 20.584 us 7 KB 7 KB 7 KB
RemoveAccount (10000, 10) - 176.514 us 147.902 us 135.926 us 26 KB 26 KB 26 KB

Test results for original PR:

Method Mean
(old Net3.1)		 Mean
(new Net6)		 Allocated
(old Net3.1)		 Allocated
(new Net6)
Serialize_MsalTokenResponse_Test 11.023 us 2.391 us 4 KB 1 KB
Deserialize_MsalTokenResponse_Test 18.934 us 5.750 us 5 KB 1 KB
Serialize_InstanceDiscoveryResponse_Test 74.784 us 22.643 us 27 KB 9 KB
Deserialize_InstanceDiscoveryResponse_Test 114.608 us 45.057 us 23 KB 14 KB
Serialize_OAuth2ResponseBase_Test 3.550 us 1.309 us 2 KB 1 KB
Deserialize_OAuth2ResponseBase_Test 6.235 us 2.404 us 4 KB 1 KB

Documentation
N/A

Code tour
systemtextjson-in-net-6.zip

@bgavrilMS
Copy link
Member

LGTM so far

@bgavrilMS
Copy link
Member

bgavrilMS commented Aug 16, 2022 •
edited
Loading

We should run some tests on net6 and some tests on netcoreapp2.1 ...
I'm guessing that this feature will be blocked until OneBranch enables net6?

@pmaytak pmaytak changed the title Pmaytak/system text json Add .NET 6 that uses System.Text.Json and IL trimming Aug 16, 2022
@pmaytak pmaytak changed the title Add .NET 6 that uses System.Text.Json and IL trimming Add .NET 6 target that uses System.Text.Json and IL trimming Aug 16, 2022
@bgavrilMS
Copy link
Member

@pmaytak - is there a noticeable different in latency / memory usage for getting a token out of the cache / getting a token from AAD ? So that we can craft an argument as "Improve perf by X% and memory usage by Y% in common scenarios"

@bgavrilMS bgavrilMS mentioned this pull request Aug 26, 2022
Closed
SameerK-MSFT
SameerK-MSFT approved these changes Aug 26, 2022
View reviewed changes
Copy link
Contributor

@SameerK-MSFT SameerK-MSFT left a comment

Choose a reason for hiding this comment

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

Please merge main and make sure that iOS Builds before you merge into main branch.

src/client/Microsoft.Identity.Client/Cache/Adal/AdalResultWrapper.cs Show resolved Hide resolved
src/client/Microsoft.Identity.Client/Cache/Items/CacheSerializationContract.cs Show resolved Hide resolved
@pmaytak pmaytak force-pushed the pmaytak/system-text-json branch from 6eefb68 to 7b921d1 Compare August 27, 2022 07:29
@pmaytak
Copy link
Contributor Author

pmaytak commented Aug 31, 2022

is there a noticeable different in latency / memory usage for getting a token out of the cache / getting a token from AAD ? So that we can craft an argument as "Improve perf by X% and memory usage by Y% in common scenarios"

@bgavrilMS Added the perf results to the top post. The NET6 STJ is definitely faster but the spread seems to be anywhere 5-50%. 10% is probably the safe number?

@pmaytak pmaytak mentioned this pull request Sep 1, 2022
Closed
@bgavrilMS
Copy link
Member

bgavrilMS commented Sep 1, 2022 •
edited
Loading

@pmaytak - just an idea, but if we were to run the perf tests on the MSAL net2.1 TFM but use net6 at runtime (e.g. by commenting out MSAL net6 TFM and running the perf tests under net6), we could actually extract / isolate the perf impact of the JSON improvement.

@bgavrilMS
Copy link
Member

Suggestion for getting the MacOs project to run - you can conditionally compile only the Xamarin targets and skip the net6 targets on that stage of the build.

@bgavrilMS
Copy link
Member

bgavrilMS commented Sep 1, 2022 •
edited
Loading

Follow up - how common is to have 1 partition / 10-1000 tokens per partition? I would think that many partitions with 1 token is more common?

@pmaytak
Copy link
Contributor Author

pmaytak commented Sep 1, 2022

@bgavrilMS

@pmaytak - just an idea, but if we were to run the perf tests on the MSAL net2.1 TFM but use net6 at runtime (e.g. by commenting out MSAL net6 TFM and running the perf tests under net6), we could actually extract / isolate the perf impact of the JSON improvement.

Hmm, yea, interesting. I can try this. This does eliminate the runtime difference, but wouldn't there still be variability with the compiled binaries themselves? To rephrase - would the same code compiled into net6 / netcore3.1 targets be different?

Follow up - how common is to have 1 partition / 10-1000 tokens per partition? I would think that many partitions with 1 token is more common?

Yea, I think many tenants with few tokens is more common. Although I remember a case or two where the Redis cache entry was large because of app token partition. I think we should add this to new telemetry. I think we already log this count anyway.

@pmaytak
Copy link
Contributor Author

pmaytak commented Sep 2, 2022

Suggestion for getting the MacOs project to run - you can conditionally compile only the Xamarin targets and skip the net6 targets on that stage of the build.

Didn't work. The issue is with restore task. It ends up using mono, which doesn't see .NET 6.

bgavrilMS
bgavrilMS approved these changes Sep 2, 2022
View reviewed changes
LibsAndSamples.sln Show resolved Hide resolved
build/platform_and_feature_flags.props Outdated Show resolved Hide resolved
@pmaytak
Copy link
Contributor Author

pmaytak commented Sep 2, 2022 •
edited
Loading

just an idea, but if we were to run the perf tests on the MSAL net2.1 TFM but use net6 at runtime (e.g. by commenting out MSAL net6 TFM and running the perf tests under net6), we could actually extract / isolate the perf impact of the JSON improvement.

@bgavrilMS

The columns are:

  • MSAL .NET Core 3.1 binary (with Newtonsoft) on .NET Core 3.1 runtime
  • MSAL .NET Core 3.1 binary (with Newtonsoft) on .NET 6 runtime
  • MSAL .NET 6 binary (with System.Text.Json) on .NET 6 runtime

Seems like there's an improvement running on .NET 6 and further improvements using STJ.

Method CacheSize EnableCacheSerialization Mean
(Net3.1 on RT Net3.1)		 Mean
(Net3.1 on RT Net6)		 Mean
(Net6 on RT Net6)		 Allocated
(Net3.1 on RT Net3.1)		 Allocated
(Net3.1 on RT Net6)		 Allocated
(Net6 on RT Net6)
Serialize_MsalTokenResponse_Test - - 6.798 us 5.693 us 1.479 us 4 KB 4 KB 2 KB
Deserialize_MsalTokenResponse_Test - - 12.807 us 9.967 us 3.167 us 5 KB 4 KB 1 KB
Serialize_InstanceDiscoveryResponse_Test - - 42.486 us 34.120 us 13.003 us 27 KB 27 KB 10 KB
Deserialize_InstanceDiscoveryResponse_Test - - 73.417 us 59.457 us 27.090 us 23 KB 17 KB 15 KB
Serialize_OAuth2ResponseBase_Test - - 2.239 us 1.813 us 1.746 us 2 KB 2 KB 1 KB
Deserialize_OAuth2ResponseBase_Test - - 4.339 us 3.273 us 1.323 us 4 KB 3 KB 1 KB
AcquireTokenForClient - - 278.705 us 270.283 us 265.245 us 70 KB 60 KB 56 KB
AcquireTokenOBO - - 367.687 us 354.133 us 342.475 us 131 KB 117 KB 100 KB
AcquireTokenForClient (1, 10) False 16.482 us 13.056 us 12.216 us 22 KB 19 KB 20 KB
AcquireTokenForOBO (1, 10) False 29.368 us 24.190 us 22.854 us 35 KB 29 KB 29 KB
AcquireTokenForClient (1, 10) True 171.427 us 148.427 us 107.139 us 271 KB 269 KB 217 KB
AcquireTokenForOBO (1, 10) True 230.642 us 384.077 us 144.363 us 326 KB 323 KB 300 KB
AcquireTokenSilent (1, 10) - 21.981 us 18.689 us 16.864 us 28 KB 25 KB 26 KB
GetAccount (1, 10) - 7.528 us 5.737 us 4.745 us 7 KB 7 KB 7 KB
RemoveAccount (1, 10) - 36.908 us 35.765 us 33.763 us 26 KB 25 KB 26 KB
AcquireTokenForClient (1, 1000) False 222.968 us 181.835 us 181.582 us 131 KB 128 KB 131 KB
AcquireTokenForOBO (1, 1000) False 270.902 us 227.717 us 217.105 us 144 KB 138 KB 144 KB
AcquireTokenForClient (1, 1000) True 26,093.618 us 23,694.945 us 13,025.475 us 18,359 KB 18,353 KB 19 KB
AcquireTokenForOBO (1, 1000) True 17,322.703 us 26,635.185 us 20,047.856 us 21,253 KB 21,162 KB 23 KB
AcquireTokenSilent (1, 1000) - 249.261 us 219.258 us 205.166 us 137 KB 134 KB 137 KB
GetAccount (1, 1000) - 7.568 us 5.832 us 4.952 us 7 KB 7 KB 7 KB
RemoveAccount (1, 1000) - 959.247 us 829.315 us 821.534 us 1,790 KB 1,789 KB 2 KB
AcquireTokenForClient (10000, 10) False 34.505 us 28.132 us 28.496 us 22 KB 19 KB 20 KB
AcquireTokenForOBO (10000, 10) False 46.846 us 38.430 us 37.620 us 35 KB 29 KB 29 KB
AcquireTokenForClient (10000, 10) True 166.482 us 157.529 us 108.532 us 271 KB 269 KB 217 KB
AcquireTokenForOBO (10000, 10) True 498.314 us 206.794 us 148.095 us 331 KB 320 KB 300 KB
AcquireTokenSilent (10000, 10) - 39.807 us 34.268 us 33.507 us 28 KB 25 KB 26 KB
GetAccount (10000, 10) - 23.566 us 21.018 us 20.584 us 7 KB 7 KB 7 KB
RemoveAccount (10000, 10) - 147.902 us 141.337 us 135.926 us 26 KB 25 KB 26 KB

@pmaytak pmaytak merged commit 08e65c7 into main Sep 2, 2022
@pmaytak pmaytak deleted the pmaytak/system-text-json branch September 2, 2022 22:39
@pmaytak pmaytak added this to the 4.47.0 milestone Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gladjohn gladjohn gladjohn approved these changes

@SameerK-MSFT SameerK-MSFT SameerK-MSFT approved these changes

@bgavrilMS bgavrilMS bgavrilMS approved these changes

@neha-bhargava neha-bhargava Awaiting requested review from neha-bhargava

@trwalke trwalke Awaiting requested review from trwalke

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.47.0
4 participants
@pmaytak @bgavrilMS @SameerK-MSFT @gladjohn