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

Remove most unnecessary resources from PNSE assemblies and partial facade assemblies #46186

Closed
wants to merge 2 commits into from

Conversation

danmoseley
Copy link
Member

@danmoseley danmoseley commented Dec 17, 2020

Fix #28888

  1. Disable resources from partial facades by default
  2. Reenable in a few projects where the PF's have some code that needs resources.
  3. Change PNSE assemblies to use a separate NotSupported.resx by default, and move their single resources there
  4. Except for a few projects where the PNSE assemblies have some code that needs resources
  5. Incidentally remove some strings named PlatformNotSupported_XX that appeared to be dead.

Other than the PNSE resources, I didn't split any Strings.resx's -- in a few cases I looked at, there were only a few strings different between different configurations, but there may be cases where there are larger differences (eg., many Windows specific strings)

Not sure how to best measure, but artifacts\packages\Debug\Shipping is about 300KB smaller

@danmoseley danmoseley changed the title asdf Remove most unnecessary resources from PNSE assemblies and partial facade assemblies Dec 17, 2020
@ghost
Copy link

ghost commented Dec 17, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #46181

  1. Disable resources from partial facades by default
  2. Reenable in a few projects where the PF's have some code that needs resources.
  3. Change PNSE assemblies to use a separate NotSupported.resx by default, and move their single resources there
  4. Except for a few projects where the PNSE assemblies have some code that needs resources
  5. Incidentally remove some strings named PlatformNotSupported_XX that appeared to be dead.

Other than the PNSE resources, I didn't split any Strings.resx's -- in a few cases I looked at, there were only a few strings different between different configurations, but there may be cases where there are larger differences (eg., many Windows specific strings)

Not sure how to best measure, but artifacts\packages\Debug\Shipping is about 300KB smaller

Author: danmosemsft
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@kouvel
Copy link
Member

kouvel commented Dec 17, 2020

Think the issue link above is incorrect, it points to my PR :)

@danmoseley danmoseley removed the request for review from ahsonkhan December 17, 2020 06:44
@danmoseley
Copy link
Member Author

Fixed, copy paste error - gosh I didn't create this very cleanly.

@danmoseley
Copy link
Member Author

Failing test fixed by #46564

@danmoseley
Copy link
Member Author

@stephentoub do you have any other feedback?

@stephentoub
Copy link
Member

Nope

@danmoseley
Copy link
Member Author

@safern @ViktorHofer perhaps one of you could review?

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

Looks reasonable. @ericstj might be interested on this change as well.

@@ -0,0 +1,63 @@
<root>
Copy link
Member

Choose a reason for hiding this comment

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

Rather than checking all this in, can we just generate it to obj? It seems these all match the same template.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good idea. I don't know why that didn't occur to me.

Copy link
Member

Choose a reason for hiding this comment

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

I suggested the resx in the issue, so it was my fault 😆

@ericstj
Copy link
Member

ericstj commented Jan 5, 2021

Not sure how to best measure, but artifacts\packages\Debug\Shipping is about 300KB smaller

That'll be compressed and won't show you the improvements you made to shared framework size. You could enumerate all dlls under artifacts\bin that are PNSE's and measure before and after when doing allconfigs build. I believe you can observe a PNSE by reading this attribute:

[assembly: AssemblyMetadata("NotSupported", "True")]

@danmoseley danmoseley marked this pull request as draft January 5, 2021 21:53
@danmoseley
Copy link
Member Author

Draftifying until I have time to do that. It will be a few days.

@ghost ghost closed this Feb 14, 2021
@ghost
Copy link

ghost commented Feb 14, 2021

Draft Pull Request was automatically closed for inactivity. It can be manually reopened in the next 30 days if the work resumes.

@danmoseley
Copy link
Member Author

There are some PNSE messages that are not formulaic. If we generate them centrally, they would become System.XXX is not supported on this platform.. I guess that is OK

eg

Access Control List (ACL) APIs are specific to the way in which Windows manages access to resources, and are not supported on this platform.
Customized reflection contexts are only supported on .NET Framework.
System.IO.Ports is currently only supported on Windows.
System.Management currently is only supported for Windows desktop applications.
System.Net.WebClient is not supported on this platform. Use System.Net.Http.HttpClient instead.
WinHttpHandler is only supported on .NET Framework and .NET runtimes on Windows. It is not supported for Windows Store Applications (UWP) or Unix platforms.

@ericstj
Copy link
Member

ericstj commented Feb 22, 2021

A property value could specify that if you want (there may already be one), or you could say that folks which want to customize the message can use the checked in resx model.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Facades and NotSupported assemblies are bloated with resources they don't use
6 participants