[WIP] Allow compilation to in memory stream, part 1: Fsc output #715

enricosada · 2015-11-06T23:03:08Z

this pr is the first of a multuple set to allow in memory compilation and output

see fsharp/fsharp-compiler-docs#266
user voice: https://fslang.uservoice.com/forums/245727-f-language/suggestions/6717137-allow-for-completely-in-memory-compilation

the refactoring plan is to start with the lower level, convert paths to stream factories and bubble up these arguments up to fsc main.
There we can build a better api for in memory compilation ( really nice for testing or compiler service i think)

current fsc main:

    main0(argv,bannerAlreadyPrinted,exiter, errorLoggerProvider, disposables)
    |> main1
    |> main2
    |> main2b
    |> main2c
    |> main3 
    |> main4

inside main0 the command line argument are parsed and outfile/pdbfile path is set and passed to all other funcions ( main1 to main4 )

this pr add output/pdb/mdb to stream or file.
The output of assembly directly to memorystream works, pdb/mdb/signed require disk io with temp file.

the api to specify where send out/pdb/mdb is

type EmitTo =
    | EmittedFile of string
    | EmittedStream of Stream

with error and deferred creation

type EmitStreamProvider = Lazy<Choice<EmitTo, Diagnostic>>
type Diagnostic = string

Choice<EmitTo, Diagnostic>:
- to use success/fail pattern instead of exception based control flow
- we can refactor (later) the control flow as computation expression
Lazy:
- deferred creation
- once
- we can ask ( Lazy.IsValueCreated ) if the factory is called, and easy cleanup the files on error
both EmittedFile of string and EmittedStream of Stream , not only Stream
- performance, useless continue to copy stream (build+sign, pdb) if i need a real file
- current implementation (pdb/sign) require file on disk, useless hide that complexity. if we later improve pdb/sign, this api is ok
- easy to consume api

As side note, roslyn do pretty much the same with EmitStreamProvider, so we can wrap later when a bridge is ready ( working on a partial bridge 😄 )

i need a feedback about naming, i think is weak (for example EmittedFile -> EmitToFile? dunno)

…llExecute true) not 260 characters

dsyme · 2015-11-07T11:51:47Z

@enricosada To clarify - this looks like an FCS feature, but you'd like the core implementation diff that enables the feature to be integrated into Microsoft/visualfsharp to keep maximal code alignment? If so I am assuming that you are confident that the feature is non-intrusive and non-risky. Also, just to check, do you think it will commute well with the CoreCLR work?

@KevinRansom If so I'm ok with the approach of commiting the core of FCS implementation feature work to Microsoft/visualfsharp first, then integrating through to FCS. If you like that approach, then we should probably bring over the core implementation diff of a couple of other FCS features like in-memory cross-project references too. That will continue to put us in a really good position to fully align in the future (e.g. having the ability to build compatible FSharp.Compiler.Service DLLs out of Microsoft/visualfsharp).

@enricosada If @KevinRansom doesn't feel able to pull in the next couple of days for stability reasons, you can submit first to FCS. Make sure the diff is absolutely minimal.

Thanks
Don

dsyme · 2015-11-07T11:53:02Z

src/absil/ilwrite.fs

@@ -222,7 +222,6 @@ type PdbData =
 //---------------------------------------------------------------------

 let WritePdbInfo fixupOverlappingSequencePoints showTimes f fpdb info = 
-    (try FileSystem.FileDelete fpdb with _ -> ())


What's this removal for?

removed because useless, the file is now removed in serializeToFile (the same for the .mdb)
I'll remove the delete inside serializeToFile and leave that one, less diff

dsyme · 2015-11-07T12:33:26Z

Added a bunch of comments - thanks! It's great to have this underway - FCS can really benefit from this feature.

enricosada · 2015-11-09T17:51:10Z

@dsyme really thx for feedback, i'll fix issues.

I wrote this internal refactoring as less intrusive as possibile, with minimal diff in mind.
I sent the pr there because i'd like a smooth integration with visualfsharp, instead of the difficult merge of multiple commit later when the whole in-memory compilation is ready in fcs.
I really like to minimize merge efforts, it'is wasted time.

I think after this one is merged in visualfsharp, fcs can pull and we can deliver the in-memory compilation without waiting visualfsharp if needed (i'd like to help too).

enricosada · 2015-11-17T17:36:18Z

src/fsharp/fsc.fs

@@ -1840,30 +1861,38 @@ let main1(tcGlobals, tcImports: TcImports, frameworkTcImports, generatedCcu, typ
      use unwindBuildPhase = PushThreadBuildPhaseUntilUnwind (BuildPhase.Output)    
      if tcConfig.printSignature   then InterfaceFileWriter.WriteInterfaceFile (tcGlobals,tcConfig, InfoReader(tcGlobals,tcImports.GetImportMap()), typedAssembly);
      ReportTime tcConfig ("Write XML document signatures")
-      if tcConfig.xmlDocOutputFile.IsSome then 
+      match xmlDocOutputOpt with
+      | Some(_) ->


converted from if to match because i need type inference, otherwise i must specify method signature for xmlDocOutputOpt param (more invasive diff)

enricosada · 2015-11-17T17:58:17Z

ok, i need to do more testing but that should be ok to review at least /cc @dsyme. I'll do more testing asap

I changed only internal functions inside fsc.fs, moving all binding one step up in the call stack (main4 -> main3 -> .. main0 ) so the previous review of the others files is ok.

i moved some string path -> EmitTo where possible (like xmldoc)

I cannot move the outfile string path upper in the call stack because i need to change TastPickle.pickleObjWithDanglingCcus signature inside EncodeOptimizationData, but this pr is already big. i can continue later with another.

btw a note, i think we need to change some members of TcConfig, like TcConfig.outputFile from string option to EmitTo, so it's easier to propagate the change from path to path or stream. Typed refactoring is really easier.

After that, we can use a wrapper of main1 as api in the compiler, so a clean api but the same internal code of the compiler, that should help compiler service (or internally the vs extensions)

I know it's seem useless now, but i really think this refactoring can help with the coreclr because now the code is easier to change (add portable pdb output as another parameter for example)

Each step main0 -> main1 -> main2 -> ... remove arguments, instead of create new ones or local bindings, so it's easier to understand.

dsyme · 2015-11-18T21:14:19Z

src/fsharp/fsc.fs

+            | ILBinaryWriter.EmitTo.Stream stream ->
+                stream.Write(resource.Bytes, 0, resource.Bytes.Length)
+            | ILBinaryWriter.EmitTo.File sigDataFileName ->
+                File.WriteAllBytes(sigDataFileName,resource.Bytes);


Please add a .WriteAllBytes instance method to ILBinaryWriter.EmitTo since this match-and-write-all-bytes is used in multiple places.

dsyme · 2016-01-21T01:05:29Z

src/fsharp/fsc.fs

-            let sigDataFileName = (Filename.chopExtension outfile)+".fsdata"
-            File.WriteAllBytes(sigDataFileName,bytes)
+            //TODO instead of bytes array we can directly write to stream, less memory pressure
+            bytes |> emitAllBytes fsDataP


FWIW it looks to me like useSignatureDataFile is always false and this is dead code, as is anything to do with fsData files.

enricosada · 2016-01-22T11:03:14Z

i'll rebase to fix emrge conflict

KevinRansom · 2016-01-23T21:47:06Z

@enricosada Enrico, in your opinion is this ready to merge?

enricosada · 2016-01-24T12:23:39Z

Yes, after rebase.

enricosada · 2016-01-24T12:24:04Z

I'll do that soon, this week

dsyme · 2016-01-25T10:26:58Z

Marked WIP until rebase is done (please remove once it is done)

dsyme · 2016-03-02T15:54:09Z

@enricosada Can you bring this up to date with CI tests passing please? It would be good to get it merged. Thanks

enricosada · 2016-03-09T09:33:31Z

@dsyme i'll wait the merge of coreclr branch inside master.
I dont want to make the merge more difficult for @KevinRansom because of pdb work

KevinRansom · 2016-04-18T19:39:03Z

@enricosada coreclr is now merged into master and pretty stable, would you like to take care of this?

Thanks

Kevin

KevinRansom · 2016-05-26T18:06:39Z

@enricosada,

Hey ... do you want to finish this off or shall I close this PR?

Thanks

Kevin

enricosada · 2016-06-06T15:15:55Z

i'll close now and reopen a new pr after a rebase, too much diff atm

enricosada added 6 commits November 6, 2015 18:08

add GetTempFilePathShim, same as System.IO.Path.GetTempFileName

b50d4a9

instead of approximate pdb path length, use MAXPATH

dc0a75c

now pdb filename is not required, replaced with a bool

67a797c

emit fsc output to stream

e438fb0

removed check because command line max length is 32K ( 8K with UseShe…

1db59c5

…llExecute true) not 260 characters

remove unused linkNativeResources tempFilePath argument

390f685

msftclas added the cla-already-signed label Nov 6, 2015

dsyme reviewed Nov 7, 2015
View reviewed changes

enricosada mentioned this pull request Nov 9, 2015

Allow compilation to in memory stream fsharp/fsharp-compiler-docs#266

Closed

enricosada added 9 commits November 17, 2015 15:46

bubble up to main1, but not outfile

abfd90a

bubble up to main0, but not outfile

da29127

extract xmlDocOutputOpt option

a7aefc8

move check of filename extension before usage instead of inside function

45ab8f1

use stream instead of file

06568be

use ILBinaryWriter.EmitTo for xmlDoc output

32ddb58

use match because i need the inference

a6fcc48

bubble up xmldoc to main0

18120d9

minimize diff

8c54320

enricosada reviewed Nov 17, 2015
View reviewed changes

dsyme reviewed Nov 18, 2015
View reviewed changes

refactor extract function

38b01c7

enricosada force-pushed the fsc_output_to_stream branch from 30d8c8c to db3a69a Compare November 27, 2015 10:28

Merge branch 'master' into fsc_output_to_stream

d415cdc

enricosada force-pushed the fsc_output_to_stream branch from db3a69a to d415cdc Compare November 27, 2015 10:52

fix build

a5216d1

dsyme reviewed Jan 21, 2016
View reviewed changes

dsyme changed the title ~~Allow compilation to in memory stream, part 1: Fsc output~~ [WIP] Allow compilation to in memory stream, part 1: Fsc output Jan 25, 2016

enricosada closed this Jun 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Allow compilation to in memory stream, part 1: Fsc output #715

[WIP] Allow compilation to in memory stream, part 1: Fsc output #715

enricosada commented Nov 6, 2015

dsyme commented Nov 7, 2015

dsyme Nov 7, 2015

enricosada Nov 9, 2015

dsyme commented Nov 7, 2015

enricosada commented Nov 9, 2015

enricosada Nov 17, 2015

enricosada commented Nov 17, 2015

dsyme Nov 18, 2015

dsyme Jan 21, 2016

enricosada commented Jan 22, 2016

KevinRansom commented Jan 23, 2016

enricosada commented Jan 24, 2016

enricosada commented Jan 24, 2016

dsyme commented Jan 25, 2016

dsyme commented Mar 2, 2016

enricosada commented Mar 9, 2016

KevinRansom commented Apr 18, 2016

KevinRansom commented May 26, 2016

enricosada commented Jun 6, 2016

[WIP] Allow compilation to in memory stream, part 1: Fsc output #715

[WIP] Allow compilation to in memory stream, part 1: Fsc output #715

Conversation

enricosada commented Nov 6, 2015

dsyme commented Nov 7, 2015

dsyme Nov 7, 2015

Choose a reason for hiding this comment

enricosada Nov 9, 2015

Choose a reason for hiding this comment

dsyme commented Nov 7, 2015

enricosada commented Nov 9, 2015

enricosada Nov 17, 2015

Choose a reason for hiding this comment

enricosada commented Nov 17, 2015

dsyme Nov 18, 2015

Choose a reason for hiding this comment

dsyme Jan 21, 2016

Choose a reason for hiding this comment

enricosada commented Jan 22, 2016

KevinRansom commented Jan 23, 2016

enricosada commented Jan 24, 2016

enricosada commented Jan 24, 2016

dsyme commented Jan 25, 2016

dsyme commented Mar 2, 2016

enricosada commented Mar 9, 2016

KevinRansom commented Apr 18, 2016

KevinRansom commented May 26, 2016

enricosada commented Jun 6, 2016