Skip to content

Move Finalize from TcImports to another object #8251

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
cartermp merged 2 commits into from
Jan 17, 2020
Merged

Move Finalize from TcImports to another object #8251

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09b6df8
Move Finalize from TcImports to another object responsible for dispos…
TIHan Jan 17, 2020
d9627e1
Use ResizeArray
TIHan Jan 17, 2020
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
60 changes: 39 additions & 21 deletions src/fsharp/CompileOps.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3756,6 +3756,38 @@ type TcConfigProvider =
// TcImports
//--------------------------------------------------------------------------

[<Sealed>]
type TcImportsSafeDisposal
(disposeActions: ResizeArray<unit -> unit>,
#if !NO_EXTENSIONTYPING
disposeTypeProviderActions: ResizeArray<unit -> unit>,
#endif
compilationThread: ICompilationThread) =

let mutable isDisposed = false

let dispose () =
// disposing deliberately only closes this tcImports, not the ones up the chain
isDisposed <- true
if verbose then
dprintf "disposing of TcImports, %d binaries\n" disposeActions.Count
#if !NO_EXTENSIONTYPING
let actions = disposeTypeProviderActions
if actions.Count > 0 then
compilationThread.EnqueueWork (fun _ -> for action in actions do action())
#endif
for action in disposeActions do action()

override _.Finalize() =
dispose ()

interface IDisposable with

member this.Dispose() =
if not isDisposed then
GC.SuppressFinalize this
dispose ()

#if !NO_EXTENSIONTYPING
// 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.
Expand Down Expand Up @@ -3800,34 +3832,24 @@ and [<Sealed>] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse
let mutable dllTable: NameMap<ImportedBinary> = NameMap.empty
let mutable ccuInfos: ImportedAssembly list = []
let mutable ccuTable: NameMap<ImportedAssembly> = NameMap.empty
let mutable disposeActions = []
let disposeActions = ResizeArray()
Copy link
Contributor

@smoothdeveloper smoothdeveloper Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This remind me of something I was looking at:

fsharp/src/fsharp/ConstraintSolver.fs

Lines 370 to 376 in 0a0889e

type Trace =
{ mutable actions: ((unit -> unit) * (unit -> unit)) list }
static member New () = { actions = [] }
member t.Undo () = List.iter (fun (_, a) -> a ()) t.actions
member t.Push f undo = t.actions <- (f, undo) :: t.actions

this is used a lot in ResolveOverloading and is probably short lived so not the same kind of usage pattern.

I was wondering about the impact of all allocations happening in ResolveOverloading as many things are based on exception objects, I guess just as a convenience to make the error message, but this is only useful when failing to resolve an overload which is a corner case rather than the default.

This happens while checking each of the candidates, and there are several passes (using trace objects), since Exception is a very large object, generally larger itself than the few references we end up needing once the resolution failed to assemble our own exceptions, I intuit that lots of allocations could be saved there, but I also intuit that the refactoring of the data structures wouldn't be a low hanging fruit.

In any case, exciting to see the work you are doing dealing with allocations in the compiler 👍.

Copy link
Contributor Author

@TIHan TIHan Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would wonder too. I've seen this kind of pattern as well in other places. It depends on what the use case is. You can think of this pattern like simply swapping state rather than changing it underneath (which is what ResizeArray) does.

For this PR in particular, using the list state swapping pattern is fine but I need a reference to the actual state without capturing TcImports inside the object doing the disposal. I would have to make this a ref cell which I didn't want, and chose ResizeArray instead.

let mutable disposed = false
let mutable ilGlobalsOpt = ilGlobalsOpt
let mutable tcGlobals = None
#if !NO_EXTENSIONTYPING
let mutable disposeTypeProviderActions = []
let disposeTypeProviderActions = ResizeArray()
let mutable generatedTypeRoots = new System.Collections.Generic.Dictionary<ILTypeRef, int * ProviderGeneratedType>()
let mutable tcImportsWeak = TcImportsWeakHack (WeakReference<_> this)
#endif

let disposal = new TcImportsSafeDisposal(disposeActions, disposeTypeProviderActions, compilationThread)

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
#if !NO_EXTENSIONTYPING
let actions = disposeTypeProviderActions
disposeTypeProviderActions <- []
if actions.Length > 0 then
compilationThread.EnqueueWork (fun _ -> for action in actions do action())
#endif
let actions = disposeActions
disposeActions <- []
for action in actions do action()
(disposal :> IDisposable).Dispose()

static let ccuHasType (ccu: CcuThunk) (nsname: string list) (tname: string) =
let matchNameSpace (entityOpt: Entity option) n =
Expand Down Expand Up @@ -4043,12 +4065,12 @@ and [<Sealed>] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse

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

#if !NO_EXTENSIONTYPING
member private tcImports.AttachDisposeTypeProviderAction action =
CheckDisposed()
disposeTypeProviderActions <- action :: disposeTypeProviderActions
disposeTypeProviderActions.Add action
#endif

// Note: the returned binary reader is associated with the tcImports, i.e. when the tcImports are closed
Expand Down Expand Up @@ -4781,9 +4803,6 @@ and [<Sealed>] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse
knownUnresolved
|> List.map (function UnresolvedAssemblyReference(file, originalReferences) -> file, originalReferences)
|> List.iter reportAssemblyNotResolved

override tcImports.Finalize () =
dispose ()

static member BuildNonFrameworkTcImports (ctok, tcConfigP: TcConfigProvider, tcGlobals: TcGlobals, baseTcImports, nonFrameworkReferences, knownUnresolved) =
cancellable {
Expand All @@ -4809,7 +4828,6 @@ and [<Sealed>] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse
interface System.IDisposable with
member tcImports.Dispose() =
dispose ()
GC.SuppressFinalize tcImports

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

Expand Down