[WIP] Reuse typechecking results - stage 1 #17898
Conversation
❗ Release notes required
Warning No PR link found in some release notes, please consider adding it.
|
psfinaki
changed the title
| for KeyValue((idx, fileName), _) in graph do | ||
| appendLine $"%i{idx}[\"%s{fileName}\"]" | ||
|
|
||
| for KeyValue((idx, _), deps) in graph do |
There was a problem hiding this comment.
We might print something different here, so far this is partially copypasted from the mermaid serialization.
tests/benchmarks/FCSBenchmarks/CompilerServiceBenchmarks/ReuseTypecheckingResultsBenchmarks.fs
Outdated
Show resolved
Hide resolved
| @@ -1961,7 +1961,54 @@ let CheckMultipleInputsUsingGraphMode | |||
|
|
|||
| partialResults, tcState) | |||
|
|
|||
| let TryReuseTypecheckingResults (tcConfig: TcConfig) inputs = | |||
| let tcDataFileName = FileSystem.GetFullPathShim "FSharpTypecheckingData" | |||
There was a problem hiding this comment.
This should probably come from msbuild, via the value of the flag, and should be set to an intermediate output
There was a problem hiding this comment.
Yeah, I need to resolve and set the proper location for the file here.
The name got inspiration for FSharpSignatureData and FSharpOptimizationData we already have, but it can be whatever.
| let existingTcData = tcDataFile.ReadAllText() | ||
|
|
||
| if thisTcData = existingTcData then | ||
| tcConfig.exiter.Exit 0 |
There was a problem hiding this comment.
In this phase, we don't cache any actual typechecking results yet... So when we detect there is no need for recompilation, there is nothing to restore and propagate yet.
So what do we do here? Just stop? That's weird.
There was a problem hiding this comment.
If the feature it cannot actually restore typechecking data, it should not do anything. Just signal a bool, ignore it, but do not exit.
Collecting metrics to actually evaluate the hit/miss ratios would be good as well.
There was a problem hiding this comment.
Well this we hit not when we cannot restore the data but we don't want to do that. Essentially, when we detect that nothing has changed since the last compilation.
If we ignore it, we will keep recompiling code in such cases. Is that what we want at this stage?
There was a problem hiding this comment.
As long as this feature cannot persist and rehydrate the typechecking results instead of recalculating them - yes, indeed.
There was a problem hiding this comment.
Mmm okay, removed the exit here. As for the metric - you mean collecting them via events?
There was a problem hiding this comment.
Some telemetry later telling us "how good" it is in terms of cache hits.
There was a problem hiding this comment.
That's clear, it's more about the mechanism to use. I only noticed events being used in the compiler which we could probably utilize. Or do you have some other idea?
There was a problem hiding this comment.
Use activities for now is good enough. Later on we'll hook up dotnet-collect compatible events
src/Compiler/Driver/fsc.fs
Outdated
| @@ -513,6 +513,7 @@ let main1 | |||
| rangeForErrors = range0 | |||
| ) | |||
|
|
|||
| tcConfigB.cmdLineArgs <- argv |> String.concat " " | |||
There was a problem hiding this comment.
So we need to keep track of the cmd line arguments here. What's the best way to propagate this along the code?
The current mechanism (via globals) forces this to be mutable - which it doesn't have to be, hence I don't like it too much. Alternative would be creating yet another bunch of optional parameters to affected functions all over the compiler - don't like that too much either...
There was a problem hiding this comment.
Only TcConfigBuilder uses mutable data, since it is a builder.
This information should be propagated to TcConfig or TcGlobals (depending on where it is needed), where things are immutable.
There was a problem hiding this comment.
No we do propagate it to TcConfig now, that's what's ParseAndCheckInputs uses now.
|
@T-Gro PTAL, this is an early stage so I mostly just wanted to discuss the questions I raised in the comments, so that we go in the agreed direction. |
| { | ||
| Idx = idx | ||
| FileName = input.FileName | ||
| ParsedInput = input |
There was a problem hiding this comment.
In general I am missing "lastupdated" timestamps for making the comparison relevant.
Also compiler version or any other header to be a quick circuit breaker in case of newer compiler getting access to older files.
(might even be version/timestamp/.. of Fsharp.Compiler.Service.dll itself I guess?)
There was a problem hiding this comment.
Correct, I added a notion of this to what we write to the TcData.
First part of the implementation of reusing typecheck results, as per the design doc.
This basically does 2 things:
--reusetypecheckingresults, with all the hookingOpen questions: see my comments.