Use Roslyn for backing metadata bytes in VS #4586

dsyme · 2018-03-20T15:38:32Z

This is a replacement for #4528

This is a solution to the "ByteFile" memory usage problem discussed here and here

It uses resources for referenced DLLs as follows :

fsc.exe: uses memory mapped files like it's always done. It releases them on process exit.
fsi.exe: uses short-lived memory mapped files to open PE files, then strongly-held ByteFile after that.
FSharp.Compiler.Service: By default, anything built with FCS uses weakly-held WeakByteFile for stable DLLs, and strongly-held ByteFile for everything else
devenv.exe: By default, Visual Studio uses the Roslyn memory manager to get metadata resources for DLLs. A short-lived memory mapped file is used to open PE files, but then it's closed again.

Much of the change is splitting our .NET binary reader into the PE reader and the Metadata reader

Background

ByteFile accounts for something like 25% of F# memory usage in Visual Studio and long running FCS applications (we have to revalidate that this is still the case, but we expect it to be)

@vbfox says:

From my measurements ByteFile is the single thing that make VS OOM for our team.
Both the compiler directly and via type providers maintain them in memory, and having a dll with some bitmap resources of a few dozen MB can trash VS memory very fast.
And from what I saw in the code only the tables (types, methods, ...) are used and not the IL or the resources or any other.

ByteFile objects come from DLL references - we read the whole DLL into memory. The memory gets collected if you move on to a new project or the builder for the project is otherwise collected. But while the project is being edited the memory is held strongly.

The memory held by byte[] is not very intrusive for GC, but is a killer when the memory reaches max address space, which affects Visual Studio in particular (most other F# editing tools use 64-bit address spaces)

Validating which storage is used

The --times flag now shows the kind of storage used and the instance count. Some asserts have also been added.

This validates that fsc.exe is using memory mapped files where possible, byte arrays elsewhere:

C:\GitHub\dsyme\visualfsharp>release\net40\bin\fsc --times a.fs
...
STATS: #ByteArrayFile = 2, #MemoryMappedFileOpen = 23, #MemoryMappedFileClosed = 0, #RawMemoryFile = 0, #WeakByteArrayFile = 0

This validates that fsi.exe is using ByteFile (the memory mapped files are temporary while cracking the files):

C:\GitHub\dsyme\visualfsharp>release\net40\bin\fsi --times
...
STATS: #ByteArrayFile = 26, #MemoryMappedFileOpen = 26, #MemoryMappedFileClosed = 26, #RawMemoryFile = 0, #WeakByteArrayFile = 0

Details for FCS docs:

By default, FCS uses a "stable name heuristic". This is not used by Visual Studio, which uses the Roslyn memory manager - we don't use this to avoid taking a dependency on Roslyn). The solution here is to hold the ByteFile memory weakly for stable DLLs:

Stable DLL = DLLs whose full path name contains "packages" or "Reference Assemblies" or "lib/mono"

FCS holds the bytes weakly, resurrects them if they are needed and have been collected, and complains if the file has been written to under its feet.

Done:

Update to use Roslyn memory manager when running in Visual Studio (i.e. FSharp.Editor.dll)
Validate via a memory dump that we have reduced active byte[] usage
Get it green
add the heuristic to the FileSystem shim

…o weak1

auduchinok · 2018-03-20T15:59:17Z

By default, FCS uses a "stable name heuristic".

Could it be added to the file system shim? Clients then could use that behaviour or override if needed.

vbfox · 2018-03-20T16:21:44Z

src/absil/ilread.fs

+let stableFileHeuristicApplies (filename:string) = 
+    try 
+       let directory = Path.GetDirectoryName(filename)
+       directory.Contains("Reference Assemblies") || directory.Contains("packages") || directory.Contains("lib/mono")


Maybe include path separators ? To avoid any project named "foopackageme" to get considered stable :)

Also is there any consequences for paket users where /packages/ dlls can change on paket install/update ?

Yes path does not change when paket updates a lib. But a gazillion of other things like file size, file version, last modified date,....

So in that sense it's not different than me maintaining a lib folder and replacing a dll in it

Yes except that the dll in the lib folder will be loaded into memory because it won't benefit from the heuristic :)

I was just wondering what is the failure case, is it just a single build in FCS that warn/fail if it build while the packages are updating or if there is more long lived consequences.

I was just wondering what is the failure case, is it just a single build in FCS that warn/fail if it build while the packages are updating or if there is more long lived consequences.

The failure would normally be an exception surfaced as a intellisense error saying "please reload"

…o weak2

dsyme · 2018-03-21T01:51:21Z

I've validated that this is using Roslyn manager to manage the memory for DLL references. There are now now ByteFile/WeakByteFile references showing in VS dumps

dsyme · 2018-03-21T01:51:44Z

Could it be added to the file system shim? Clients then could use that behaviour or override if needed.

Yes, I will do that

dsyme · 2018-03-21T03:46:16Z

src/absil/ilread.fs

@@ -3184,215 +3276,34 @@ let getPdbReader opts infile =
          with e -> dprintn ("* Warning: PDB file could not be read and will be ignored: "+e.Message); None         
 #endif

-//-----------------------------------------------------------------------


This big change is shifting around code to split the PE reader and IL metadata reader

dsyme · 2018-03-21T03:46:51Z

src/absil/ilread.fs

+    let ilModule, ilAssemblyRefs, pdb = openPE (fileName, pefile, opts.pdbPath, (opts.reduceMemoryUsage = ReduceMemoryFlag.Yes), opts.ilGlobals)
+    new ILModuleReader(ilModule, ilAssemblyRefs, (fun () -> ClosePdbReader pdb))
+
+let OpenILModuleReader fileName opts = 


This is the routine that makes decisions about how to open a binary

chinwobble · 2018-03-21T11:52:38Z

@dsyme how do you read the memory dump of the all objects and their types like that?

Is it from the "Debug Managed Memory" that appears from a minidump file, which is only available in VS Enterprise? Is it possible to read it without VS Enterprise?

dsyme · 2018-03-21T13:11:29Z

@dsyme how do you read the memory dump of the all objects and their types like that? Is it from the "Debug Managed Memory" that appears from a minidump file, which is only available in VS Enterprise? Is it possible to read it without VS Enterprise?

Yes, I'm using VS Enterprise. I think you can use windbg to get a similar dump - not quite as convenient but i've done it in the past https://blogs.msdn.microsoft.com/paullou/2011/06/28/debugging-managed-code-memory-leak-with-memory-dump-using-windbg/

AviAvni · 2018-03-21T13:41:08Z

@chinwobble look at #4580 with clrmd you can read managed heap of dump with code also you can attach to live proccess

…o weak2

dsyme · 2018-03-21T20:57:39Z

This is ready

dsyme · 2018-03-22T03:31:30Z

@KevinRansom Requesting a review. It's a large change but is mostly just plumbing

The Roslyn metadata service is accessed here https://github.com/Microsoft/visualfsharp/pull/4586/files#diff-cdf0a4fbb7615b8029d78133924259c3R51

I've been using this all day in VS and not seen any problems.

smoothdeveloper · 2018-03-22T04:03:24Z

src/fsharp/CompileOps.fs

      mutable projectReferences : IProjectReference list
      mutable knownUnresolvedReferences : UnresolvedAssemblyReference list
-      optimizeForMemory: bool
+      reduceMemoryUsage: ReduceMemoryFlag


Here might be a good location to give overview of what this flag implies in the implementation.

Will also help when updating https://fsharp.github.io/ in context of those optimization.

Sorry, missed that it is done in the .fsi...

Yes, there are docs here https://github.com/Microsoft/visualfsharp/pull/4586/files#diff-3835db36ed701c86797a3c95c330f3daR51 but the Compiler Guide is an important place to update too

dsyme · 2018-03-22T13:07:27Z

I'll merge this, since it is needed as a basis for #4596. I've been through and added some further comments.

* weak ByteFile * cleanup, only use in VS * cleanup flags * some comments * some comments * use Roslyn memory manager for metadata in VS * report statistics only with --times, clarify flags2 * minor updates * us in VS * fix build * fix build * fix build * add SFH to FileSystem * fix build * fix build * fix build * fix build * add some comments

dsyme added 9 commits March 15, 2018 03:26

weak ByteFile

c020a4b

cleanup, only use in VS

ab7ff10

Merge branch 'master' of http://github.com/Microsoft/visualfsharp int…

9054129

…o weak1

cleanup flags

58bd2be

Merge branch 'master' of http://github.com/Microsoft/visualfsharp int…

e8df380

…o weak1

some comments

b77cc23

some comments

6be11a0

use Roslyn memory manager for metadata in VS

b027b58

report statistics only with --times, clarify flags2

77834ec

vbfox reviewed Mar 20, 2018

View reviewed changes

dsyme added 2 commits March 20, 2018 16:47

minor updates

4d8ae2c

Merge branch 'master' of http://github.com/Microsoft/visualfsharp int…

8368e3e

…o weak2

This was referenced Mar 20, 2018

Warnings during build-from-VisualStudio #4541

Closed

[CompilerPerf] Weak ByteFile[] #4528

Closed

us in VS

445c883

fix build

da18505

dsyme changed the title ~~Use Roslyn memory manager for backing metadata bytes in VS~~ [CompilerPerf] Use Roslyn for backing metadata bytes in VS Mar 21, 2018

dsyme commented Mar 21, 2018

View reviewed changes

fix build

35b7e2c

fix build

0446dbd

dsyme added 3 commits March 21, 2018 13:51

add SFH to FileSystem

dfc44ca

fix build

a852565

fix build

071b2d4

dsyme added 2 commits March 21, 2018 18:06

fix build

b982dbb

Merge branch 'master' of http://github.com/Microsoft/visualfsharp int…

695b38a

…o weak2

dsyme mentioned this pull request Mar 21, 2018

Reduce resident memory for VS tokenization and other caches #4590

Merged

2 tasks

Merge branch 'master' into weak2

bd49ff5

Merge branch 'master' into weak2

78eb92c

fix build

a68d742

smoothdeveloper reviewed Mar 22, 2018

View reviewed changes

add some comments

8b4d6ef

dsyme changed the title ~~[CompilerPerf] Use Roslyn for backing metadata bytes in VS~~ Use Roslyn for backing metadata bytes in VS Mar 22, 2018

dsyme merged commit 75e435e into dotnet:master Mar 22, 2018

This was referenced Mar 22, 2018

Read/store ILCustomAttrs, ILSecurityDecls, ILTypeDef sensibly #4597

Merged

Fix FCS problem with duplicate ByteFile #4629

Merged

auduchinok mentioned this pull request Mar 28, 2018

Reduce FCS memory footprint #4258

Closed

10 tasks

KevinRansom mentioned this pull request Apr 28, 2018

Fix FSC : error FS0193: Could not find file 'c:\Users\kevinr\source\repos\Library2\Library2\tmpB4E3.dll #4800

Merged

Use Roslyn for backing metadata bytes in VS #4586

Use Roslyn for backing metadata bytes in VS #4586

Uh oh!

Conversation

dsyme commented Mar 20, 2018 • edited by cartermp Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Validating which storage is used

Details for FCS docs:

Uh oh!

auduchinok commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

dsyme commented Mar 21, 2018

Uh oh!

dsyme commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinwobble commented Mar 21, 2018

Uh oh!

dsyme commented Mar 21, 2018

Uh oh!

AviAvni commented Mar 21, 2018

Uh oh!

dsyme commented Mar 21, 2018

Uh oh!

dsyme commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dsyme commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dsyme commented Mar 20, 2018 •

edited by cartermp

Loading

auduchinok commented Mar 20, 2018 •

edited

Loading

dsyme commented Mar 22, 2018 •

edited

Loading