Skip to content

Removed reference counting from IncrementalBuilder; now relies on GC. #6863

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.

Sign up for GitHub

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

Merged
TIHan merged 11 commits into from
May 31, 2019
Merged

Removed reference counting from IncrementalBuilder; now relies on GC. #6863

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cc41ebe
TcImports doesn't explicitly have to be disposed, relies on GC. Remov…
TIHan May 23, 2019
5779215
Removing any notion of being incremental builder reference counting a…
TIHan May 24, 2019
9251353
Added ITypeProviderThread
TIHan May 25, 2019
b3d0b38
Fixed declaration list item
TIHan May 25, 2019
fb8ecf7
Changed EnqueueWorkAndWait to just EnqueueWork that's not blocking
TIHan May 25, 2019
84308e5
Minor cleanup
TIHan May 25, 2019
2a693f8
Forgot to check disposed
TIHan May 25, 2019
05b7d39
Quick update on comment
TIHan May 25, 2019
e11699d
Update FCS project
TIHan May 25, 2019
a6c6306
Renamed ITypeProviderThread to ICompilationThread
TIHan May 30, 2019
0262e2d
Changes from feedback and added a test
TIHan May 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
TcImports doesn't explicitly have to be disposed, relies on GC. Remov… 
…ed reference counting in incremental builder. Builders can only be invalidated by a type provider, not anything else.
  • Loading branch information
@TIHan
TIHan committed May 23, 2019
commit cc41ebec98af9e0660f2acfe84b3e6463291f59c
82 changes: 61 additions & 21 deletions src/fsharp/CompileOps.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3741,10 +3741,42 @@ type TcConfigProvider =
// TcImports
//--------------------------------------------------------------------------

// These are hacks in order to allow TcImports to be held as a weak reference inside a type provider.
// The reason is due to older type providers compiled using an older TypeProviderSDK, that SDK used reflection on fields and properties to determine the contract.
// The reflection code has now since been removed, see here: https://github.com/fsprojects/FSharp.TypeProviders.SDK/pull/305. But we still need to work on older type providers.
// One day we can remove these hacks when we deemed most if not all type providers were re-compiled using the newer TypeProviderSDK.
// Yuck.
type DllInfoHack =
{
FileName: string
}

and TcImportsWeakHack (tcImports: WeakReference<TcImports>) =
let mutable dllInfos: DllInfoHack list =
match tcImports.TryGetTarget () with
| true, strong -> strong.GetDllInfos () |> List.map (fun x -> { FileName = x.FileName })
| _ -> []

member __.NonUse_dllInfos = dllInfos

member __.Base: TcImportsWeakHack option =
match tcImports.TryGetTarget() with
| true, strong ->
match strong.Base with
| Some baseTcImports ->
Some (TcImportsWeakHack (WeakReference<_> baseTcImports))
| _ ->
None
| _ ->
None

member __.SystemRuntimeContainsType typeName =
match tcImports.TryGetTarget () with
| true, strong -> strong.SystemRuntimeContainsType typeName
| _ -> false

/// Represents a table of imported assemblies with their resolutions.
[<Sealed>]
type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolutions, importsBase: TcImports option, ilGlobalsOpt) =
and [<Sealed>] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolutions, importsBase: TcImports option, ilGlobalsOpt) =

let mutable resolutions = initialResolutions
let mutable importsBase: TcImports option = importsBase
Expand All @@ -3763,6 +3795,16 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
let CheckDisposed() =
if disposed then assert false

let dispose () =
CheckDisposed()
// disposing deliberately only closes this tcImports, not the ones up the chain
disposed <- true
if verbose then
dprintf "disposing of TcImports, %d binaries\n" disposeActions.Length
let actions = disposeActions
disposeActions <- []
for action in actions do action()

static let ccuHasType (ccu: CcuThunk) (nsname: string list) (tname: string) =
let matchNameSpace (entityOpt: Entity option) n =
match entityOpt with
Expand All @@ -3777,7 +3819,7 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
| None -> false
| None -> false

member private tcImports.Base =
member internal tcImports.Base =
CheckDisposed()
importsBase

Expand All @@ -3800,7 +3842,7 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
dllInfos <- dllInfos ++ dllInfo
dllTable <- NameMap.add (getNameOfScopeRef dllInfo.ILScopeRef) dllInfo dllTable

member tcImports.GetDllInfos() =
member tcImports.GetDllInfos() : ImportedBinary list =
CheckDisposed()
match importsBase with
| Some importsBase-> importsBase.GetDllInfos() @ dllInfos
Expand Down Expand Up @@ -3967,7 +4009,7 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
|> Seq.toList
#endif

member tcImports.AttachDisposeAction action =
member private tcImports.AttachDisposeAction action =
CheckDisposed()
disposeActions <- action :: disposeActions

Expand Down Expand Up @@ -4119,7 +4161,7 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu

| _ -> failwith "Unexpected representation in namespace entity referred to by a type provider"

member tcImports.ImportTypeProviderExtensions
member tcImportsStrong.ImportTypeProviderExtensions
(ctok, tcConfig: TcConfig,
fileNameOfRuntimeAssembly,
ilScopeRefOfRuntimeAssembly,
Expand Down Expand Up @@ -4153,17 +4195,18 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
{ resolutionFolder = tcConfig.implicitIncludeDir
outputFile = tcConfig.outputFile
showResolutionMessages = tcConfig.showExtensionTypeMessages
referencedAssemblies = Array.distinct [| for r in tcImports.AllAssemblyResolutions() -> r.resolvedPath |]
referencedAssemblies = Array.distinct [| for r in tcImportsStrong.AllAssemblyResolutions() -> r.resolvedPath |]
temporaryFolder = FileSystem.GetTempPathShim() }

// The type provider should not hold strong references to disposed
// TcImport objects. So the callbacks provided in the type provider config
// dispatch via a thunk which gets set to a non-resource-capturing
// failing function when the object is disposed.
let systemRuntimeContainsType =
// NOTE: do not touch this
// NOTE: do not touch this, edit: but we did, we had no choice - TPs cannot hold a strong reference on TcImports "ever".
let tcImports = TcImportsWeakHack (WeakReference<_> tcImportsStrong)
let systemRuntimeContainsTypeRef = ref (fun typeName -> tcImports.SystemRuntimeContainsType typeName)
tcImports.AttachDisposeAction(fun () -> systemRuntimeContainsTypeRef := (fun _ -> raise (System.ObjectDisposedException("The type provider has been disposed"))))
tcImportsStrong.AttachDisposeAction(fun () -> systemRuntimeContainsTypeRef := (fun _ -> raise (System.ObjectDisposedException("The type provider has been disposed"))))
fun arg -> systemRuntimeContainsTypeRef.Value arg

let providers =
Expand All @@ -4174,7 +4217,7 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
// Note, type providers are disposable objects. The TcImports owns the provider objects - when/if it is disposed, the providers are disposed.
// We ignore all exceptions from provider disposal.
for provider in providers do
tcImports.AttachDisposeAction(fun () ->
tcImportsStrong.AttachDisposeAction(fun () ->
try
provider.PUntaintNoFailure(fun x -> x).Dispose()
with e ->
Expand Down Expand Up @@ -4203,7 +4246,7 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
let handler = tp.Invalidate.Subscribe(fun _ -> capturedInvalidateCcu.Trigger (capturedMessage))

// When the TcImports is disposed we detach the invalidation callback
tcImports.AttachDisposeAction(fun () -> try handler.Dispose() with _ -> ())), m)
tcImportsStrong.AttachDisposeAction(fun () -> try handler.Dispose() with _ -> ())), m)

match providers with
| [] ->
Expand All @@ -4222,7 +4265,7 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
// for that namespace.
let rec loop (providedNamespace: Tainted<IProvidedNamespace>) =
let path = ExtensionTyping.GetProvidedNamespaceAsPath(m, provider, providedNamespace.PUntaint((fun r -> r.NamespaceName), m))
tcImports.InjectProvidedNamespaceOrTypeIntoEntity (typeProviderEnvironment, tcConfig, m, entityToInjectInto, [], path, provider, None)
tcImportsStrong.InjectProvidedNamespaceOrTypeIntoEntity (typeProviderEnvironment, tcConfig, m, entityToInjectInto, [], path, provider, None)

// Inject entities for the types returned by provider.GetTypes().
//
Expand All @@ -4232,7 +4275,7 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
let tys = providedNamespace.PApplyArray((fun provider -> provider.GetTypes()), "GetTypes", m)
let ptys = [| for ty in tys -> ty.PApply((fun ty -> ty |> ProvidedType.CreateNoContext), m) |]
for st in ptys do
tcImports.InjectProvidedNamespaceOrTypeIntoEntity (typeProviderEnvironment, tcConfig, m, entityToInjectInto, [], path, provider, Some st)
tcImportsStrong.InjectProvidedNamespaceOrTypeIntoEntity (typeProviderEnvironment, tcConfig, m, entityToInjectInto, [], path, provider, Some st)

for providedNestedNamespace in providedNamespace.PApplyArray((fun provider -> provider.GetNestedNamespaces()), "GetNestedNamespaces", m) do
loop providedNestedNamespace
Expand Down Expand Up @@ -4666,6 +4709,9 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu
knownUnresolved
|> List.map (function UnresolvedAssemblyReference(file, originalReferences) -> file, originalReferences)
|> List.iter reportAssemblyNotResolved

override tcImports.Finalize () =
dispose ()

// Note: This returns a TcImports object. TcImports are disposable - the caller owns the returned TcImports object
// and when hosted in Visual Studio or another long-running process must dispose this object.
Expand Down Expand Up @@ -4696,14 +4742,8 @@ type TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAssemblyResolu

interface System.IDisposable with
member tcImports.Dispose() =
CheckDisposed()
// disposing deliberately only closes this tcImports, not the ones up the chain
disposed <- true
if verbose then
dprintf "disposing of TcImports, %d binaries\n" disposeActions.Length
let actions = disposeActions
disposeActions <- []
for action in actions do action()
dispose ()
GC.SuppressFinalize tcImports

override tcImports.ToString() = "TcImports(...)"

Expand Down
87 changes: 15 additions & 72 deletions src/fsharp/service/IncrementalBuild.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,7 +1211,9 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
keepAssemblyContents, keepAllBackgroundResolutions, maxTimeShareMilliseconds) =

let tcConfigP = TcConfigProvider.Constant tcConfig
#if !NO_EXTENSIONTYPING
let importsInvalidated = new Event<string>()
#endif
let fileParsed = new Event<string>()
let beforeFileChecked = new Event<string>()
let fileChecked = new Event<string>()
Expand Down Expand Up @@ -1242,26 +1244,9 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
for (_, f, _) in sourceFiles do
yield f |]

// The IncrementalBuilder needs to hold up to one item that needs to be disposed, which is the tcImports for the incremental
// build.
let mutable cleanupItem = None: TcImports option
let disposeCleanupItem() =
match cleanupItem with
| None -> ()
| Some item ->
cleanupItem <- None
dispose item

let setCleanupItem x =
assert cleanupItem.IsNone
cleanupItem <- Some x

let mutable disposed = false
let assertNotDisposed() =
if disposed then
System.Diagnostics.Debug.Assert(false, "IncrementalBuild object has already been disposed!")

let mutable referenceCount = 0
#if !NO_EXTENSIONTYPING
let mutable isInvalidatedByTypeProviders = false
#endif

//----------------------------------------------------
// START OF BUILD TASK FUNCTIONS
Expand All @@ -1270,14 +1255,12 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
///
/// Get the timestamp of the given file name.
let StampFileNameTask (cache: TimeStampCache) _ctok (_m: range, filename: string, _isLastCompiland) =
assertNotDisposed()
cache.GetFileTimeStamp filename

/// This is a build task function that gets placed into the build rules as the computation for a VectorMap
///
/// Parse the given file and return the given input.
let ParseTask ctok (sourceRange: range, filename: string, isLastCompiland) =
assertNotDisposed()
DoesNotRequireCompilerThreadTokenAndCouldPossiblyBeMadeConcurrent ctok

let errorLogger = CompilationErrorLogger("ParseTask", tcConfig.errorSeverityOptions)
Expand All @@ -1300,7 +1283,6 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
///
/// Timestamps of referenced assemblies are taken from the file's timestamp.
let StampReferencedAssemblyTask (cache: TimeStampCache) ctok (_ref, timeStamper) =
assertNotDisposed()
timeStamper cache ctok


Expand All @@ -1309,18 +1291,13 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
// Link all the assemblies together and produce the input typecheck accumulator
let CombineImportedAssembliesTask ctok _ : Cancellable<TypeCheckAccumulator> =
cancellable {
assertNotDisposed()
let errorLogger = CompilationErrorLogger("CombineImportedAssembliesTask", tcConfig.errorSeverityOptions)
// Return the disposable object that cleans up
use _holder = new CompilationGlobalsScope(errorLogger, BuildPhase.Parameter)

let! tcImports =
cancellable {
try
// We dispose any previous tcImports, for the case where a dependency changed which caused this part
// of the partial build to be re-evaluated.
disposeCleanupItem()

let! tcImports = TcImports.BuildNonFrameworkTcImports(ctok, tcConfigP, tcGlobals, frameworkTcImports, nonFrameworkResolutions, unresolvedReferences)
#if !NO_EXTENSIONTYPING
tcImports.GetCcusExcludingBase() |> Seq.iter (fun ccu ->
Expand All @@ -1342,15 +1319,8 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
ccu.Deref.InvalidateEvent.Add(fun msg ->
match capturedImportsInvalidated.TryGetTarget() with
| true, tg -> tg.Trigger msg
| _ -> ()))
| _ -> ()))
#endif


// The tcImports must be cleaned up if this builder ever gets disposed. We also dispose any previous
// tcImports should we be re-creating an entry because a dependency changed which caused this part
// of the partial build to be re-evaluated.
setCleanupItem tcImports

return tcImports
with e ->
System.Diagnostics.Debug.Assert(false, sprintf "Could not BuildAllReferencedDllTcImports %A" e)
Expand Down Expand Up @@ -1390,7 +1360,6 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
///
/// Type check all files.
let TypeCheckTask ctok (tcAcc: TypeCheckAccumulator) input: Eventually<TypeCheckAccumulator> =
assertNotDisposed()
match input with
| Some input, _sourceRange, filename, parseErrors->
IncrementalBuilderEventTesting.MRU.Add(IncrementalBuilderEventTesting.IBETypechecked filename)
Expand Down Expand Up @@ -1462,7 +1431,6 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
/// Finish up the typechecking to produce outputs for the rest of the compilation process
let FinalizeTypeCheckTask ctok (tcStates: TypeCheckAccumulator[]) =
cancellable {
assertNotDisposed()
DoesNotRequireCompilerThreadTokenAndCouldPossiblyBeMadeConcurrent ctok

let errorLogger = CompilationErrorLogger("CombineImportedAssembliesTask", tcConfig.errorSeverityOptions)
Expand Down Expand Up @@ -1568,7 +1536,11 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
// END OF BUILD DESCRIPTION
// ---------------------------------------------------------------------------------------------

do IncrementalBuilderEventTesting.MRU.Add(IncrementalBuilderEventTesting.IBECreated)
do
IncrementalBuilderEventTesting.MRU.Add(IncrementalBuilderEventTesting.IBECreated)
#if !NO_EXTENSIONTYPING
importsInvalidated.Publish.Add (fun _ -> isInvalidatedByTypeProviders <- true)
#endif

let buildInputs = [ BuildInput.VectorInput (fileNamesNode, sourceFiles)
BuildInput.VectorInput (referencedAssembliesNode, nonFrameworkAssemblyInputs) ]
Expand All @@ -1581,21 +1553,11 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput
partialBuild <- b

let MaxTimeStampInDependencies cache (ctok: CompilationThreadToken) (output: INode) =
IncrementalBuild.MaxTimeStampInDependencies cache ctok output.Name partialBuild

member this.IncrementUsageCount() =
assertNotDisposed()
System.Threading.Interlocked.Increment(&referenceCount) |> ignore
{ new System.IDisposable with member __.Dispose() = this.DecrementUsageCount() }

member __.DecrementUsageCount() =
assertNotDisposed()
let currentValue = System.Threading.Interlocked.Decrement(&referenceCount)
if currentValue = 0 then
disposed <- true
disposeCleanupItem()
IncrementalBuild.MaxTimeStampInDependencies cache ctok output.Name partialBuild

member __.IsAlive = referenceCount > 0
#if !NO_EXTENSIONTYPING
member __.IsInvalidatedByTypeProviders = isInvalidatedByTypeProviders
#endif

member __.TcConfig = tcConfig

Expand All @@ -1611,17 +1573,6 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput

member __.AllDependenciesDeprecated = allDependencies

#if !NO_EXTENSIONTYPING
member __.ThereAreLiveTypeProviders =
let liveTPs =
match cleanupItem with
| None -> []
| Some tcImports -> [for ia in tcImports.GetImportedAssemblies() do yield! ia.TypeProviders]
match liveTPs with
| [] -> false
| _ -> true
#endif

member __.Step (ctok: CompilationThreadToken) =
cancellable {
let cache = TimeStampCache defaultTimeStamp // One per step
Expand Down Expand Up @@ -1887,11 +1838,3 @@ type IncrementalBuilder(tcGlobals, frameworkTcImports, nonFrameworkAssemblyInput

return builderOpt, diagnostics
}

static member KeepBuilderAlive (builderOpt: IncrementalBuilder option) =
match builderOpt with
| Some builder -> builder.IncrementUsageCount()
| None -> { new System.IDisposable with member __.Dispose() = () }

member __.IsBeingKeptAliveApartFromCacheEntry = (referenceCount >= 2)

Loading