Proof of concept: reusing typecheck results #18354
Conversation
❗ Release notes requiredCaution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository:
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request. You can open this PR in browser to add release notes: open in github.dev
|
| @@ -2781,6 +2781,8 @@ type ILTypeDef | |||
|
|
|||
| member _.MetadataIndex = metadataIndex | |||
|
|
|||
| member _.Flags = additionalFlags | |||
There was a problem hiding this comment.
This is expected, basic APIs will likely have to increase their surface to make them accessible to the new pickling code.
|
|
||
| let mutable state = tcState | ||
|
|
||
| let results = |
There was a problem hiding this comment.
So this is apparently a very dumb mechanism of collecting states. Instead, the state "deltas" should be collected and merged here.
| @@ -239,7 +255,7 @@ val CheckClosedInputSet: | |||
| tcState: TcState * | |||
| eagerFormat: (PhasedDiagnostic -> PhasedDiagnostic) * | |||
| inputs: ParsedInput list -> | |||
| TcState * TopAttribs * CheckedImplFile list * TcEnv | |||
| TcState * TopAttribs * CheckedImplFile list * TcEnv * TcState list | |||
There was a problem hiding this comment.
Also note, TcState list includes the first state. I just kept the first state separately here to decrease the change surface a bit for now.
| @@ -0,0 +1,355 @@ | |||
| module internal FSharp.Compiler.ReuseTcResults.CachingDriver | |||
|
|
|||
| #nowarn "3261" | |||
There was a problem hiding this comment.
Nullness warning, don't remove it, just fix it.
| type CachingDriver(tcConfig: TcConfig) = | ||
|
|
||
| let outputDir = tcConfig.outputDir |> Option.defaultValue "" | ||
| let tcDataFilePath = Path.Combine(outputDir, "tcComplilationData") |
There was a problem hiding this comment.
A bunch of files... should eventually have some better structure and naming.
|
|
||
| let GetSharedData (file, ilScopeRef, ilModule, byteReaderA, byteReaderB) = | ||
|
|
||
| let memA = byteReaderA () |
There was a problem hiding this comment.
This is really just copypaste from existing imports... we likely don't need to byte readers and all that stuff can be simplified.
|
|
||
| let p_stamp_map pv = p_Map p_stamp pv | ||
|
|
||
| let p_non_null_slot f (x: 'a | null) st = |
There was a problem hiding this comment.
This is a terrible hack based on top of another terrible hack of even having these slots. This should be eliminated.
It's current used to artificially "backfill" the bindings, instead they should be also cached and restored normally.
It's not that easy to do though. That often leads to SO due to recursion here, or too much data stored - and so on.
| let p_ILTypeDef (x: ILTypeDef) st = | ||
| p_string x.Name st | ||
| //p_type_attributes x.Attributes | ||
| //p_il_type_def_layout x.Layout |
There was a problem hiding this comment.
Eventually commented things here should be also pickled and unpickled.
| (p_tcref "ucref") | ||
| p_string | ||
| (p_non_null_slot p_entity_spec_new) | ||
| (a, b, a.binding) |
There was a problem hiding this comment.
As noted earlier, bindings should not be pickled separately... instead, the type references should be pickled in a way that they contain all the necessary information to auto-restore the bindings.
| let tyconRef, typeInstantiation, nullness, binding = | ||
| u_tup4 | ||
| u_tcref | ||
| u_tys_new |
There was a problem hiding this comment.
That's really very, very hard. I haven't come up with a mechanism to properly cache and restore tcref. The compiler needs different info about the entity on different stages (optimizations, tail call checks, code gen, whatever...). And goes through things like typar solutions and referenced entities. But storing it all fully leads to StackOverflows usually...
| @@ -473,6 +551,8 @@ let main1 | |||
| disposables: DisposablesTracker | |||
| ) = | |||
|
|
|||
| CompilerGlobalState.stampCount <- 0L | |||
There was a problem hiding this comment.
Horrible hack to keep stamps in tact... instead, the whole state should be cached at the right moment and restored along with the stamps.
| @@ -802,7 +802,11 @@ let check (ilscope: ILScopeRef) (inMap: NodeInTable<_,_>) = | |||
| for i = 0 to inMap.Count - 1 do | |||
| let n = inMap.Get i | |||
| if not (inMap.IsLinked n) then | |||
| warning(Error(FSComp.SR.pickleMissingDefinition (i, inMap.Name, ilscope.QualifiedName), range0)) | |||
|
|
|||
| // TODO: do not disable | |||
There was a problem hiding this comment.
This otherwise creates thousands of warnings... should be fixed properly.
There was a problem hiding this comment.
Deleted temporarily just to not fill in all the signatures...
| @@ -464,6 +463,9 @@ | |||
| <Content Include="Driver\GraphChecking\Docs.md" /> | |||
| <Compile Include="Driver\ParseAndCheckInputs.fsi" /> | |||
| <Compile Include="Driver\ParseAndCheckInputs.fs" /> | |||
| <Compile Include="Driver\ReuseTcResults\TcResultsPickle.fs" /> | |||
There was a problem hiding this comment.
Those should have signatures as well.
This a POC for this feature: #17260
Design doc is here: https://github.com/dotnet/fsharp/blob/main/docs/reusing-typechecking-results.md
The PR shows that the idea is viable, the tests contain examples of the feature working ever for multiple files, where new files use the results of the previous compilation. However, it is far from a proper implementation and, as of now, is supposed to take more time that we can give it. Therefore the effort will be hanging here for a while, welcoming any help to push it through.
See TODOs, comments and other notes. The testing is kind of in place, there are some seemingly unrelated old tests failing, needs investigation as well. An overview of big things not implemented yet:
tcref) without StackOverflows