Singlefile compression of native files by VSadov · Pull Request #49855 · dotnet/runtime

VSadov · 2021-03-19T04:54:19Z

First part of implementing compression in singlefile apps.

In this change:

introducing compression support in Bundler and annotation for compressed files in the header.
the scheme is used for native an symbol files (compressing managed assemblies is not in this change)
decompression support in extractor.
additional option for Bundler to toggle support for compression.
Current use is only in tests. Eventually SDK will use this flag, also plans for exposing the flag in msbuild.
current default is no-compression. What default will be chosen in SDK is TBD.

ghost · 2021-03-19T04:54:25Z

Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

In progress, just running tests

Author:	VSadov
Assignees:	-
Labels:	`area-Single-File`
Milestone:	-

VSadov · 2021-03-23T01:00:04Z

I think this PR has everything that I planned for this part.
@vitek-karas could you take another look? Thanks!

vitek-karas · 2021-03-23T10:50:38Z

            Version targetFrameworkVersion = null)
        {
            UseSingleFileSelfContainedHost(testFixture);
+            options |= BundleOptions.EnableCompression;


We should also add a couple of tests which disable compression on purpose. I know we expect compression to be on by default, but will allow users to disable it, so we should validate that it works.

Some files are not compressed, and .json never will be, so the case with uncompressed files is validated.
Turning off compression intentionally in one of self-contained tests is an easy thing to do though.

VSadov · 2021-03-27T15:29:49Z

@vitek-karas - I think I have addressed all the concerns. Let me know if I missed something.

vitek-karas

Some comments, but nothing major... looks good!

vitek-karas · 2021-03-29T10:16:40Z

 using System.IO;
 using System.Reflection.PortableExecutable;
 using System.Runtime.InteropServices;
+using System.IO.Compression;


Nit: sort usings

vitek-karas · 2021-03-29T10:21:29Z

+            UseSingleFileSelfContainedHost(fixture);
+            options |= BundleOptions.EnableCompression;
            var singleFile = BundleHelper.BundleApp(fixture, options);


Given that we plan to have this on by default - I would probably change the default test behavior as well. Make the BundleHelper turn on compression by default and have a way for the test to selectively disable it.

From the perspective of the Bundler compression will be off by default. That is because the Bundler does not know if we use framework-dependent bundle and it would be a dangerous default.
I think BundleHelper should default to "off" for the same reasons. Compression must be an opt-in feature at the bundler level. SDK can have different defaults since it has more information.

I agree to a point - basically if I add a new test - I will definitely forget to set this option - and thus my test will not cover the most common scenario. Given the history of bad and few tests in this area I'd like to make it so that we have at least reasonable coverage for the most common cases.

I have explored making UseSingleFileSelfContainedHost to enable compression automatically, but it was not very natural since the “fixture” stuff operates at proj file level and does not know much about the bundle.

Perhaps I should revisit that direction. Maybe set a flag on the fixture, that BundleHelper would read.

Nope going further that direction only make things worse. If UseSingleFileSelfContainedHost sets "can do compression" flag, then there are two ways to specify whether compression is desired - because fixture says so and via flags passed to bundler. The flag on the fixture must take precedence and that is a bit unintuitive and could be error prone if fixture is reused. Also for the unusual/error cases I would need another argument to override the override. It gets messy.

Here what seems to work better.

Add a helper BundleSelfContainedApp which both picks the right host and then adds compression flags and passes that to Bundler.

Make all tests that could use BundleSelfContainedApp use it - that is nearly all tests that do singlefile stuff.

A a few tests that need to tests unusual/error combinations go the long route - they will have to use UseSingleFileSelfContainedHost and explicitly and have spell out what they need in terms of compression to BundleApp. There are only a few tests like that.

I would expect the obvious difference in verbosity will direct future testcase writers towards the "default" path.

vitek-karas · 2021-03-29T10:25:52Z

@mateoatr could you please also take a look. It's a relatively major change so it would be best to have multiple people look at it. I'm especially nervous about the versioning and compat behaviors (always a tricky topic).

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

mateoatr · 2021-03-29T17:19:55Z

        {
            var fixture = sharedTestState.TestSelfContainedFixture.Copy();
+            UseSingleFileSelfContainedHost(fixture);
+            options |= BundleOptions.EnableCompression;


Should we have something similar in NetCoreApp3CompatModeTests?

Per @vitek-karas suggestion I am making compression a default behavior in all singlefile tests. Only few tests will explicitly use singlefilehost and no compression.
I think it will take care of this as well.

mateoatr

LGTM

VSadov · 2021-03-29T23:09:26Z

Thanks!!

ghost · 2021-03-29T23:13:45Z

Hello @VSadov!

Because this pull request has the auto-merge 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.

jjxtra · 2021-03-30T14:24:26Z

Adding lzma compression in the future would be interesting, further reducing size at the cost of slightly longer build time and slightly longer decompression times.

VSadov added 3 commits March 18, 2021 21:31

versioning

ae2af40

no need to support 2.0 bundle version

812707c

enable compression

853bd9a

ghost added the area-Single-File label Mar 19, 2021

fixes after rebase

0c1b756

VSadov force-pushed the compression branch from 405344b to 0c1b756 Compare March 19, 2021 06:30

vitek-karas reviewed Mar 19, 2021

View reviewed changes

Comment thread src/installer/managed/Microsoft.NET.HostModel/Bundle/Bundler.cs

vitek-karas reviewed Mar 19, 2021

View reviewed changes

Comment thread src/installer/managed/Microsoft.NET.HostModel/Bundle/Bundler.cs Outdated

vitek-karas reviewed Mar 19, 2021

View reviewed changes

Comment thread src/installer/managed/Microsoft.NET.HostModel/Bundle/FileEntry.cs Outdated

VSadov added 5 commits March 19, 2021 10:17

build fix for Unix

84e3795

Fix build with GCC

10775b6

disable a test temporarily to see what else breaks

88c23b2

Add EnableCompression option to Bundler + PR feedback

05398d2

Couple more places should use version 6.0

dcacd81

VSadov marked this pull request as ready for review March 22, 2021 20:02

vitek-karas requested changes Mar 23, 2021

View reviewed changes

PR feedback (header versioning, more tests, fixed an assert)

2649177

vitek-karas approved these changes Mar 29, 2021

View reviewed changes

vitek-karas requested a review from mateoatr March 29, 2021 10:24

VSadov and others added 3 commits March 29, 2021 08:13

Suggestion from PR review

15d4b61

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

sorted usings

19b3bbc

should be bundle_major_version in two more places.

7727976

mateoatr reviewed Mar 29, 2021

View reviewed changes

Comment thread src/installer/managed/Microsoft.NET.HostModel/Bundle/Bundler.cs

mateoatr reviewed Mar 29, 2021

View reviewed changes

Comment thread src/native/corehost/bundle/extractor.cpp Outdated

mateoatr reviewed Mar 29, 2021

View reviewed changes

Comment thread src/native/corehost/bundle/extractor.cpp Outdated

mateoatr reviewed Mar 29, 2021

View reviewed changes

More PR feedback

92cec71

VSadov requested a review from mateoatr March 29, 2021 23:00

mateoatr approved these changes Mar 29, 2021

View reviewed changes

VSadov added the auto-merge label Mar 29, 2021

ghost merged commit 857fe5b into dotnet:main Mar 29, 2021

VSadov deleted the compression branch March 29, 2021 23:17

This was referenced Mar 30, 2021

Compress single file .exe embedded assemblies dotnet/sdk#13736

Closed

Compression support for single-file publish #44569

Closed

vitek-karas mentioned this pull request Apr 19, 2021

Support for single-file bundle from .NET 6. icsharpcode/ILSpy#2373

Merged

ghost locked as resolved and limited conversation to collaborators Apr 29, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

This pull request was closed.

Uh oh!

Conversation

VSadov commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VSadov commented Mar 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Mar 25, 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!

VSadov commented Mar 27, 2021

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VSadov Mar 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vitek-karas commented Mar 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateoatr left a comment

Choose a reason for hiding this comment

Uh oh!

VSadov commented Mar 29, 2021

Uh oh!

ghost commented Mar 29, 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!

jjxtra commented Mar 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

VSadov commented Mar 19, 2021 •

edited

Loading

VSadov Mar 25, 2021 •

edited

Loading

VSadov Mar 29, 2021 •

edited

Loading

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.