Split Microsoft.Toolkit.Diagnostics package #3726

Sergio0694 · 2021-02-03T01:52:57Z

Related to #3594, working towards reducing footprint and increasing modularity

PR Type

What kind of change does this PR introduce?

Refactoring (no functional changes, no api changes)

What is the current behavior?

The Guard and ThrowHelper APIs are part of the Microsoft.Toolkit package.
This has a few downsides with respect to binary size for consuming package and overall API surface for consumers.

The Guard and ThrowHelper APIs relied on a number of different NuGet packages that needed to be pulled in:

System.Diagnostics.Contracts
System.ValueTuple
System.Memory
System.Runtime.CompilerServices.Unsafe

These packages are only needed by those APIs, and yet with Microsoft.Toolkit being referenced by every other package in the whole Toolkit, every other package ended up having a dependency on those as well even when there was no use for them at all. This is not ideal both for final binary size, for the increased number of potentially unwanted APIs being imported by consumers, and due to the fact that many consumers want to minimize the number of NuGet dependencies in general.

Furthermore, the .Diagnostics APIs can be very useful for all kinds of developers, especially those working in backend scenarios and with a particular focus on performance. It was not ideal to force those developers to also import APIs such as observable collections and other more UI-oriented APIs that were available in the base package (in fact I've spoken with a few devs that didn't like having to include anything other than just the Guard and ThrowHelper APIs when adding the package).

For consumers on .NET 5 that don't use .NET Native, splitting the Guard and ThrowHelper APIs away means 80KB less in binary size (when not using IL trimming) when those APIs are not needed, which is effectively a 75% reduction on the base package.

What is the new behavior?

The Guard and ThrowHelper APIs have now been moved to a new Microsoft.Toolkit.Diagnostics package.
Splitting this package completely removes all those dependencies from all other packages too 🚀
This work will also make it easier in the future to introduce a .Diagnostics source-only package, if we wanted to.

Note: marking as breaking change due to the APIs being moved to a different package, but this PR doesn't do any changes to the overall API surface across the various packages - the APIs are still all exactly the same, just modularized differently.

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
~~Pull Request has been submitted to the documentation repository instructions. Link:~~
~~Sample in sample app has been added / updated (for bug fixes / features)~~
- ~~Icon has been created (if new sample) following the Thumbnail Style Guide and templates~~
New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

ghost · 2021-02-03T01:53:16Z

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

Sergio0694 · 2021-02-03T12:36:36Z

Alright, CI is passing now and the smoke test is running fine too! 🥳

`Microsoft.Toolkit` from main branch

Microsoft.Toolkit Additional Max Footprint: 128,772 bytes
Microsoft.Toolkit Additional Min Footprint: 426,638 bytes
  App Diff: (.exe) = 1,024
  App Diff: (.xml) = 638
  App Diff: (.appxsym) = 18,494
  Size Diff: AppxBundleManifest.xml = 6
  Size Diff: AppxBlockMap.xml = 228
  Size Diff: AppxManifest.xml = 6
  Size Diff: resources.pri = 192
  Size Diff: System.Runtime.CompilerServices.Unsafe.dll = 1,536
  Lib (self): Microsoft.Toolkit.dll = 106,648
-----------------COMPILED----------------
  App Diff: (.dll) = 425,984
  App Diff: (.exe) = 0
  Size Diff: AppxBundleManifest.xml = 6
  Size Diff: AppxBlockMap.xml = 450
  Size Diff: AppxManifest.xml = 6
  Size Diff: resources.pri = 192
-----------------------------------------

`Microsoft.Toolkit` from this PR

Microsoft.Toolkit Additional Max Footprint: 37,671 bytes
Microsoft.Toolkit Additional Min Footprint: 126,314 bytes
  App Diff: (.exe) = 1,024
  App Diff: (.xml) = 638
  App Diff: (.appxsym) = 9,027
  Size Diff: AppxBundleManifest.xml = 6
  Size Diff: AppxBlockMap.xml = 154
  Size Diff: AppxManifest.xml = 6
  Size Diff: resources.pri = 192
  Lib (self): Microsoft.Toolkit.dll = 26,624
-----------------COMPILED----------------
  App Diff: (.dll) = 125,952
  App Diff: (.exe) = 0
  Size Diff: AppxBundleManifest.xml = 6
  Size Diff: AppxBlockMap.xml = 158
  Size Diff: AppxManifest.xml = 6
  Size Diff: resources.pri = 192
-----------------------------------------

It's basically 1/4th of the size, and we also ditch all the extra indirect dependencies for consumers both in terms of less final binary size, both in terms of them having extra dependencies they might not want, and extra APIs they might not need.
And at the same time, consumers only wanting to use the Guard and ThrowHelper APIs now get a single dedicated package as well, so they won't have to also get the other non-diagnostics APIs that are in the base package, so win-win! 😄

Rosuavio · 2021-02-03T20:50:45Z

I am seeing some errors when I run the tests in VisualStudio but nothing when I run the CI scripts. 🤔

michael-hawker

This is awesome, thanks @Sergio0694! 🎉🎉🎉

Amazing to see the impact up the chain in the Toolkit, and excited to explore some of the Source stuff in the future and understand that world, but this is a great first step.

It should also make us feel a bit more comfortable at putting some basic helpers in the .NET world for the Toolkit that we use upwards in the Toolkit, as I think splitting any of those chains up is pretty hard.

ghost · 2021-02-03T20:57:43Z

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

michael-hawker · 2021-02-03T20:58:31Z

@RosarioPulella if this gets merged before we're done dev/split-controls, we should probably leave it out of dev/split-controls so we can compare against the rest of the numbers we've been running. But I think we're getting closer to wrapping that all up soon, so I'm not too worried overall yet.

michael-hawker · 2021-02-03T21:01:13Z

@msftbot merge when @RosarioPulella approves

ghost · 2021-02-03T21:01:17Z

Hello @michael-hawker!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @rosariopulella

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

Sergio0694 · 2021-02-03T21:19:53Z

I am seeing some errors when I run the tests in VisualStudio but nothing when I run the CI scripts. 🤔

I'm seeing those two in the TokenizingTextBox:

I think the reason why the CI doesn't detect them is because those tests are under an #if DEBUG, so they're skipped.

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/058c896a1c4fdfe92084c8d07a9e234992ed5213/Microsoft.Toolkit.Uwp.UI.Controls/TokenizingTextBox/InterspersedObservableCollection.cs#L197-L207

This PR shouldn't have affected the behavior there though, so I think it's possible they might've been failing before too?

FYI @michael-hawker

michael-hawker · 2021-02-03T22:04:33Z

Hmm... I thought I had run all the Unit Tests locally recently and they were fine. Let me check on the main branch to compare. I'm trying to remember why those were Debug only now though... I think I just did it for perf/initial testing.

Rosuavio · 2021-02-03T23:05:48Z

Just tested master its not having the same issues.

michael-hawker · 2021-02-04T00:11:32Z

Yup, I had a few issues building main at first, but then was able to run the Unit Tests with no problems. @Sergio0694 ideas?

Sergio0694 · 2021-02-04T00:19:22Z

Let me try main on my end too and then I'll double check my changes here to make sure I didn't just slip an incorrect refactor in one of those tests - I might've just not noticed it at first due to those paths not being executed by the CI, so that'd explain it 🤔

Sergio0694 · 2021-02-04T00:31:02Z

@michael-hawker All good, fixed in d61cd24!
I had indeed read a check backwards and inverted the condition there, which was causing the test to fail, sorry! 😅
We should be good to go now! 🚀

@RosarioPulella Thanks for spotting that!

michael-hawker · 2021-02-04T00:48:37Z

Microsoft.Toolkit.Uwp.UI.Controls/TokenizingTextBox/InterspersedObservableCollection.cs

@@ -193,8 +195,15 @@ private void ReadjustKeys()
        private int ToInnerIndex(int outerIndex)
        {
 #if DEBUG
-            Guard.IsInRange(outerIndex, 0, Count, nameof(outerIndex));
-            Guard.IsFalse(_interspersedObjects.ContainsKey(outerIndex), nameof(outerIndex)); // We can't map a inserted key to the original collection!
+            if ((uint)outerIndex >= Count)


Do we have to worry about the lower-bound check here that was originally done by IsInRange or is that handled by the (uint) conversion?

The lower bound is handled by the (uint) cast, it's the same trick I use in the Guard APIs as well to skip one conditional branch and checks both edges of the range with just one. This works because signed integers are stored in 2-complemement, with the most significant bit being the sign. If you cast to uint instead, if the value was negative, it'd just underflow and go to int.MaxValue + x - 1, which will always be greater than the other comparand since that's guaranteed to be a non-negative value in the int range 🙂

michael-hawker · 2021-02-04T00:50:56Z

@Sergio0694 awesome, I've done that before too! Though it was converting a C++ statement to C# for the Gaze code... 😋

And that's why we have unit tests, nice catch @RosarioPulella!

@Sergio0694 would it be better to use ThrowHelper type logic here to keep the guards at runtime for the CI but not bog down the methods? Though I think part of my concern was that I wasn't entirely sure if I had missed any edge cases and didn't want to take down the control in the event of a problem, but I did also try to test this pretty heavily because it's such a unique collection type... Thoughts?

Sergio0694 · 2021-02-04T00:59:51Z

"would it be better to use ThrowHelper type logic here to keep the guards at runtime for the CI but not bog down the methods?"

Generally speaking I'd say it would depend on where the input is coming from - if it's from a user-provided value it would be a good idea to keep the tests, whereas if the value is coming from some other internal component within the codebase, keeping them in Debug only is fine. As in, so that the checks kept in Release would effectively only be to validate inputs and not to check for correctness in the internal implementation.

That said, the overhead of the whole control is so much greater than what those checks could possibly do that I think adding them in Release would never be noticeable anyway, so it's absolutely fine if you'd like to add them there as well. And of course we could still use some local throw helper methods anyway to keep the codegen cleaner. 🙂

michael-hawker · 2021-02-04T06:25:59Z

@Sergio0694 yeah, considering we accidently broke the test, but wouldn't have caught it in CI worried me a bit. Mind you I don't expect us to be touching this code too much.

If you'd like to update it for the more internal ThrowHelper approach, please feel free and remove the #debug flags. I know it'd just take you a second compared to me fumbling with it... 😋

Sergio0694 · 2021-02-04T14:35:27Z

@michael-hawker Done in 6a33532 😄

Rosuavio

Looks good, Thanks @Sergio0694 for going the extra bit for this one.

Sergio0694 added 4 commits February 2, 2021 23:00

Split Microsoft.Toolkit.Diagnostics package

44a0fa0

Fixed Diagnostics dependencies in other packages

90c6f40

Fixed .csproj files for base and Diagnostics packages

d6c44da

Refactored remaining dependencies in the Toolkit

a86d4bd

Sergio0694 added introduce breaking changes 💥 maintenance ⚙️ nuget 📦 .NET Components which are .NET based (non UWP specific) labels Feb 3, 2021

Sergio0694 added this to the 7.0 milestone Feb 3, 2021

ghost added the in progress 🚧 label Feb 3, 2021

ghost requested review from michael-hawker, azchohfi, Kyaa-dost and Rosuavio February 3, 2021 01:53

Sergio0694 added 2 commits February 3, 2021 12:05

Added missing references to test projects

e7d166f

Added Diagnostics skoke test project

058c896

michael-hawker approved these changes Feb 3, 2021

View reviewed changes

michael-hawker added the auto merge ⚡ label Feb 3, 2021

Fixed T4 linked files in .csproj file

659dbb0

Fixed an incorrect argument check

d61cd24

michael-hawker reviewed Feb 4, 2021

View reviewed changes

Moved InterspersedObservableCollection tests to Release

6a33532

Rosuavio approved these changes Feb 4, 2021

View reviewed changes

ghost merged commit 5a8482b into CommunityToolkit:master Feb 4, 2021

Sergio0694 deleted the feature/source-only-diagnostics branch February 4, 2021 17:18

Sergio0694 mentioned this pull request Feb 6, 2021

Review namespaces and folder structure coupling #3422

Closed

This pull request was closed.

Split Microsoft.Toolkit.Diagnostics package #3726

Split Microsoft.Toolkit.Diagnostics package #3726

Uh oh!

Conversation

Sergio0694 commented Feb 3, 2021

Related to #3594, working towards reducing footprint and increasing modularity

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Uh oh!

ghost commented Feb 3, 2021

Uh oh!

Sergio0694 commented Feb 3, 2021

Microsoft.Toolkit from main branch

Microsoft.Toolkit from this PR

Uh oh!

Rosuavio commented Feb 3, 2021

Uh oh!

michael-hawker left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 3, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

michael-hawker commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-hawker commented Feb 3, 2021

Uh oh!

ghost commented Feb 3, 2021

Uh oh!

Sergio0694 commented Feb 3, 2021

Uh oh!

michael-hawker commented Feb 3, 2021

Uh oh!

Rosuavio commented Feb 3, 2021

Uh oh!

michael-hawker commented Feb 4, 2021

Uh oh!

Sergio0694 commented Feb 4, 2021

Uh oh!

Sergio0694 commented Feb 4, 2021

Uh oh!

michael-hawker Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

Sergio0694 Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-hawker commented Feb 4, 2021

Uh oh!

Sergio0694 commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-hawker commented Feb 4, 2021

Uh oh!

Sergio0694 commented Feb 4, 2021

Uh oh!

Rosuavio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

`Microsoft.Toolkit` from main branch

`Microsoft.Toolkit` from this PR

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

michael-hawker commented Feb 3, 2021 •

edited

Loading

Sergio0694 Feb 4, 2021 •

edited

Loading

Sergio0694 commented Feb 4, 2021 •

edited

Loading