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

Change SystemInformation from static to regular class #3168

Conversation

vgromfeld
Copy link
Contributor

@vgromfeld vgromfeld commented Mar 16, 2020

This PR is a first step to address #3145.
It reduces the toolkit impact on a blank application by ~3.5 MB (x86, release)

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

SystemInformation is a static class. It contains a static LocalObjectStorageHelper instance.
This static "root" prevents the .Net Native compiler from removing this code from the generated code when not used. LocalObjectStorageHelper is pulling JSON.Net which has a big impact on the generated code size.

What is the new behavior?

SystemInformation has been changed to a regular class. It will be the responsibility of the user to use it as a singleton if needed. This approach allows us to reduce the toolkit impact on the applications when SystemInformation is not used.

This is a breaking change.

before after
app.dll 7,949 4,488

Size in Kb, x86 release build, no compilation directive

PR Checklist

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository
  • Sample in sample app has been added / updated (for bug fixes / features)
    • Icon has been created (if new sample)
  • 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

Other information

The test code. The zip contains two builds of the library (before/after) the proposed change and two blank apps with a dependency on the library but not consuming anything from it.
TestApp.zip

@ghost
Copy link

ghost commented Mar 16, 2020

Thanks vgromfeld 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 🙌

@ghost ghost assigned Kyaa-dost Mar 16, 2020
Copy link
Contributor

@azchohfi azchohfi left a comment

Choose a reason for hiding this comment

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

This is GREAT! Thanks!

@michael-hawker michael-hawker added this to the 7.0 milestone Mar 16, 2020
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Think we may just want to go the full path of providing the Singleton pattern ourselves no?

Also, we should make sure this is based off the dev/7.0.0 branch as it'd be a breaking change.

@ghost
Copy link

ghost commented Mar 17, 2020

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@azchohfi azchohfi changed the base branch from master to dev/7.0.0 March 17, 2020 22:25
@azchohfi
Copy link
Contributor

We need to merge master on 7.0.

Copy link
Contributor

@skendrot skendrot left a comment

Choose a reason for hiding this comment

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

Why are radial gauge changes part of this PR?

@michael-hawker
Copy link
Member

@skendrot apparently our 7.0 branch is out of date, so I'm going to investigate this and hopefully once we update, this gets cleaned-up again.

@michael-hawker
Copy link
Member

Alright, I updated the 7.0 branch, but I think there's still another change bleeding through? @azchohfi I'm not seeing the option on GitHub showing the PR branch is out of date and need to update again from the dev/7.0.0 branch? Maybe @vgromfeld needs to reset this manually?

Copy link
Contributor

@skendrot skendrot left a comment

Choose a reason for hiding this comment

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

Everything looks good except for the merge issues

@vgromfeld
Copy link
Contributor Author

vgromfeld commented Mar 19, 2020

@michael-hawker I will do a manual rebase on 7.0 branch to keep only my commits.

Edit: Done

@vgromfeld vgromfeld force-pushed the u/vgromfeld/binSizeReduction branch from 58e9874 to 35c617d Compare March 19, 2020 21:19
@skendrot
Copy link
Contributor

Looks good. Has documentation been updated?

@azchohfi azchohfi requested a review from michael-hawker March 20, 2020 00:30
Copy link
Contributor

@azchohfi azchohfi left a comment

Choose a reason for hiding this comment

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

Loved it. Does this still reduces the toolkit impact on a blank application by ~3.5 MB (x86, release)?

@vgromfeld
Copy link
Contributor Author

Loved it. Does this still reduces the toolkit impact on a blank application by ~3.5 MB (x86, release)?

@azchohfi Yes. The impact is the same.

@vgromfeld
Copy link
Contributor Author

vgromfeld commented Mar 20, 2020

Looks good. Has documentation been updated?

@skendrot I've created Update SystemInformation documentation in the doc repository.

@michael-hawker I'm not seeing any dev/7 branch in the documentation repository. Should we create one ?

@michael-hawker
Copy link
Member

@vgromfeld thanks! I've created a new doc branch, commented on that PR. I need to investigate the doc build failing still though.

@michael-hawker michael-hawker merged commit 673a3c0 into CommunityToolkit:dev/7.0.0 Mar 20, 2020
@michael-hawker
Copy link
Member

Thanks @vgromfeld for this! Let us know if you find any other similar optimizations we can do in other places too!

vgromfeld added a commit to vgromfeld/WindowsCommunityToolkit that referenced this pull request Mar 24, 2020
Change SystemInformation from static to regular class (CommunityToolkit#3168)
@Kyaa-dost Kyaa-dost added bug 🐛 An unexpected issue that highlights incorrect behavior and removed needs attention 👋 labels Jun 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior improvements ✨ introduce breaking changes 💥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants