-
Notifications
You must be signed in to change notification settings - Fork 831
Implement compression for sig and optdata #13671
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
Conversation
|
The proposed policy seems spot on |
|
So, we won't use memory mapped files for in-memory sigdata anymore? |
|
Also, curious what's the file size difference for FSharp.Core before and after this change? |
|
3mb before 2.1 after. i can get exact when i get to my desktop. |
Well the file will contain compressed data. once decompressed it can't be memory mapped by definition :-) |
Yeah, but I think we were using it all the way around, instead of storing it entirely in memory, we were using mmaped files (so, pretty much offloading it to the filesystem). Not that it matters much anyway. Compiler probably might use a bit more memory now. Few megs at the worst. |
|
@KevinRansom Have you measured the performance changes when reading compressed metadata of a bigger assembly? @safesparrow This could probably be an interesting case for the performance testing framework you've been working on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As below, this is a needless inner function, remove in the same way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
@ dsyme, thanks for the feedback, It never really occurred to me that resources and optimization might be separately required, but I agree it is conceivable, so retaining the existing two switches and adding a compressextrastuff switch seems like a better design. |
To directly answer your question: no The motivation for adding the feature is for situations where size on disk or download is the dominating concern. I expect that performance would inform our choice of either defaults or guidance. For now and the next year we are not changing the defaults, although that is over concern for compatibility not performance. That said, this PR does change what we do with the components built in this repo. Notably FCS will now use compressed metadata, we do need to explore the performance implications of this. My intuition is that :
It should be noted that access of this metadata is only a compile time activity. Applications built using our tools will not use it. Unless they are themselves tools built on FCS to do compilation. We are certainly open to switching FCS back to uncompressed if there is a noticeable performance degrade, although what we deliver to the SDK will likely remain compressed because there is pressure on size from there for sure, unless of course there is a significant performance degrade. I hope this clears things up. |
19414da to
cf9a87d
Compare
dsyme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a couple of minor things, plus comments from @0101
| </PropertyGroup> | ||
|
|
||
| <!-- The FSharp.Core dll provides a referencable public interface --> | ||
| <PropertyGroup Condition="'$(Configuration)' != 'Proto' and '$(CompressAllMetadata)' != 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct. Shouldn't this be'$(CompressAllMetadata)' == 'true'
| </PropertyGroup> | ||
|
|
||
| <!-- The FSharp.DependencyManager.Nuget dll does not provide a referencable public interface although it's used for testing --> | ||
| <PropertyGroup Condition="'$(Configuration)' != 'Proto'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have '$(CompressAllMetadata)' == 'true'?
|
|
||
| <!-- The FSharp.Compiler.Interactive.Settings dll provides a referencable public interface to tool builders --> | ||
| <PropertyGroup Condition="'$(Configuration)' != 'Proto'"> | ||
| <CompressMetadata>true</CompressMetadata> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct. Should this be false unconditionally?
|
|
||
| <!-- The FSharp.Compiler.Service dll provides a referencable public interface for tool builders --> | ||
| <PropertyGroup Condition="'$(Configuration)' != 'Proto'"> | ||
| <CompressMetadata>true</CompressMetadata> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look correct. Should this be false unconditionally? That is this component never uses compression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual projects correct it. By and large FSharp.Core is the only one we want compressed eventually. But we are in a chicken and egg situation.
However, tomorrows Preview vs will support compression fine.
|
Have you tested different compression algorithms? There are more alternatives in |
The F# compiler adds compile time metadata and optimization data to built assemblies.
This option adds two new compiler switches which control the production of this data, and adds compression functionality.
The switches are:
The options are:
Compression of this data can cause a large decrease in working set consumed by assemblies at runtime.
For example:
Not that this is important but fsc.exe drops from 29K downto 16K, which was kinda unexpected to me at least.
In order to verify the correct behavior throughout of fsharp.core and compiler service being compiled with compressed metadata, this pr currently builds with compression on.
We need to discuss and to decide what to do about the following:
However, we probably can't turn it on for FSharp.Core by default ... projects built with older compilers will not be able to consume an F# Core or any other dll for that matter containing compressed metadata. Currently I propose shipping with non-compressed fsharp.core for 1 year and then switching over to compressed metadata. Although net 7.0 would be a great point to switch over too, that's basically the next release.
I propose from the release of the net 7.0 SDK we compress FSharp.Compiler.Service metadata. The fcs developer community is fairly small and use up to date tools. So hopefully it wouldn't inconvenience them too much.
By default the compiler will embed raw files as it does today. In 1 year change the defaults to embed compressed files.
Apply compression settings to individual files to match the policies above.
Add a test leg, ensuring that a compressed FSharp.Core and libraries work.