Consider putting System.Text.Encoding.CodePages in the shared framework

[nguerrera]

There are currently 6 copies in SDK. See below.

It seems to be used quite a lot, and it seems to be purpose defeating that every app/component is carrying it. I think we're making apps significantly bigger more than we're benefiting from shared framework being approximately 1% smaller.

Also, it seems when you deploy as portable (no RID), you get 3 copies so 2.4 MB instead if 800 KB.

(Now there's an architectural issue in the SDK where we're publishing portable components into the toolset, then crossgening everything. This is causing excess copies of dlls like this in the SDK, which is actually tied to a single TFM and RID when all is said and done. This is hard to fix though we could hack it by deleting some files.)

C:\Program Files\dotnet\sdk\3.0.100-preview7-012271>dir /s System.Text.Encoding.CodePages.dll
 Volume in drive C is C
 Volume Serial Number is F0AA-6065

 Directory of C:\Program Files\dotnet\sdk\3.0.100-preview7-012271

06/05/2019  08:52 PM           876,624 System.Text.Encoding.CodePages.dll
               1 File(s)        876,624 bytes

 Directory of C:\Program Files\dotnet\sdk\3.0.100-preview7-012271\Roslyn\bincore

06/05/2019  08:52 PM           877,160 System.Text.Encoding.CodePages.dll
               1 File(s)        877,160 bytes

 Directory of C:\Program Files\dotnet\sdk\3.0.100-preview7-012271\Roslyn\bincore\runtimes\win\lib\netcoreapp2.0

06/05/2019  08:52 PM           880,720 System.Text.Encoding.CodePages.dll
               1 File(s)        880,720 bytes

 Directory of C:\Program Files\dotnet\sdk\3.0.100-preview7-012271\runtimes\win\lib\netcoreapp2.0

06/05/2019  08:52 PM           882,248 System.Text.Encoding.CodePages.dll
               1 File(s)        882,248 bytes

 Directory of C:\Program Files\dotnet\sdk\3.0.100-preview7-012271\Sdks\Microsoft.NET.Sdk.Razor\tools\netcoreapp3.0

05/15/2018  01:29 PM           758,928 System.Text.Encoding.CodePages.dll
               1 File(s)        758,928 bytes

 Directory of C:\Program Files\dotnet\sdk\3.0.100-preview7-012271\Sdks\Microsoft.NET.Sdk.Razor\tools\netcoreapp3.0\runtimes\win\lib\netcoreapp2.0

05/15/2018  01:29 PM           761,488 System.Text.Encoding.CodePages.dll
               1 File(s)        761,488 bytes

     Total Files Listed:
               6 File(s)      5,037,168 bytes

[nguerrera]

@ericstj @tarekgh

[nguerrera]

What's the difference between lib\netstandard2.0\System.Text.Encoding.CodePages.dll and runtimes*\System.Text.Encoding.CodePages.dll? It is quite a penalty on portable apps to need multiple copies of this 800 KB assembly.

[nguerrera]

In 2.1 and 2.2 it was also in the ASP.NET shared framework.

[tarekgh]

What's the difference between lib\netstandard2.0\System.Text.Encoding.CodePages.dll and runtimes*\System.Text.Encoding.CodePages.dll? It is quite a penalty on portable apps to need multiple copies of this 800 KB assembly.

the runtime version has a specific platform calls (i.e. the Windows runtime version is calling into kernel32 mainly for getting the default system codepage). while netstandard is not doing that and just return null as couldn't get the default codepage. maybe we can consider instead of cross compile the library, to do the magic during the runtime by detecting we are running on Windows and then do the interop at that time.

System.Text.Encoding.CodePages.dll not included in the apps by default to save the size of the apps which doesn't need such encoding. most of the times apps don't really need this encoding but as you mentioned ASP.NET needed it anyway so it include it in the ASP.NET shared framework. there is other products like Roslyn also including it as it needed it.

I don't mind moving this library to the shared framework if no one else has any concern about that.

[nguerrera]

Do they all carry identical data? If you keep it out of the framework, I would suggest figuring out a way to not put the data in more than one dll in runtimes/. This is because portable apps have to carry multiple copies. Or you could do the OS specific logic using light-up.

It's one thing to say that apps should carry it, but it seems unfortunate that apps are carrying more than one copy right now.

I still think it should be in the framework, but if there's pushback, that would be my next position.

[ericstj]

It will likely reduce the size of the SDK to move it to the shared framework. Granted this will grow the runtime installer, but I wonder if we could find ways to shrink that elsewhere (certainly not part of this issue, but I bet I can find 800KB if I go hunting). I'm supportive of this change. I believe the primary reason this functionality was split off was to help .NETNative's linker strip it from apps. I think it may have been accidental to exclude it from the shared framework.

I think folks that really care about size will go standalone and use the linker. The linker is friendly to this API since it requires explicit registration if you are using it (assuming you aren't using something like ASP.NET that registers it for you).

I wonder how this decision might impact our thoughts around future .NET 5 library sharing? My guess is that shared framework size isn't as important as CoreLib size, but its worth asking @marek-safar.

[tarekgh]

Do they all carry identical data?

Yes

If you keep it out of the framework, I would suggest figuring out a way to not put the data in more than one dll in runtimes/. This is because portable apps have to carry multiple copies. Or you could do the OS specific logic using light-up.

Yes, that is what I suggest too.

I still think it should be in the framework,

Looks no one objecting so far :-)

[marek-safar]

It seems to be used quite a lot

Do you have any data backing this up? nuget.org does not show this package as to be super popular.

I wonder how this decision might impact our thoughts around future .NET 5 library sharing?

I agree that the installed shared framework size is not as important as application size. It's also quite likely we'll grow the shared framework size for .NET 5.

[tarekgh]

Looks everyone agreeing we should move the library to the shared framework.

[tarekgh]

@jkotas @stephentoub any concern regarding moving this library to the shared framework?

[jkotas]

I am fine with it.

[stephentoub]

Seems ok.

[tarekgh]

thanks all. I'll go ahead with the implementation.