clear reader cache in background to prevent leak

[dsyme]

@TIHan This eliminates the long-term memory leak from the IL readers used by each TP implementation by clearing the reader cache every 30 seconds since last access (at the potential cost of re-computing readers).

[dsyme]

@TIHan Not sure the best way to unit test this. Perhaps an internal-only test hook

[cartermp]

I get that this is an improvement, but it feels like we're just sort of throwing something at the wall and hoping it sticks here. Is there a way to tie this to the lifetime of the FSharpChecker instance used in tooling instead?

[cartermp]

What's the significance of 30 seconds compared with other values? When we consider that FCS re-checks frequently, how often would this re-allocate the large amount of data (400+ MB) on the LOH in normal usage?

[TIHan]

Re testing: Yes, we would need to find an internal hook, similar to how I did testing before with the #if directives.

While this is to prevent memory leaks, it may not actually prevent it as each time we fetch assembly DLLs, we read a full byte [] which can allocate on the LOH. Over time, this can keep expanding the LOH depending on how bad the fragmentation is. Perhaps, we should address that issue in a separate PR.

I agree with @cartermp - it would be nice if we had some sort of interface that would allow us to hook into the lifetime of metadata from outside the TP (such as Roslyn). If that isn't feasible, then this may be the best we got.

[dsyme]

Re testing: Yes, we would need to find an internal hook, similar to how I did testing before with the #if directives.

OK will add this

While this is to prevent memory leaks, it may not actually prevent it as each time we fetch assembly DLLs, we read a full byte [] which can allocate on the LOH. Over time, this can keep expanding the LOH depending on how bad the fragmentation is. Perhaps, we should address that issue in a separate PR.

Yes, agreed. Will add separate issue for use of a big byte[]

I agree with @cartermp - it would be nice if we had some sort of interface that would allow us to hook into the lifetime of metadata from outside the TP (such as Roslyn). If that isn't feasible, then this may be the best we got.

Yes indeed, agreed. Perhaps we can discuss as part of #297. This issue really says "how can the TP know about the target context" - at the moment the only information communicated is the awful reflection hack for the list of referenced assemblies.

[cartermp]

@dsyme see here for issue on the byte[] array: dotnet/fsharp#5931

[dsyme]

@TIHan @cartermp I updated this so that it uses both a weak and strong cache

1. the weak cache ensures sharing while a type provider is active or while anything else is keeping the reader instance alive. The weak cache is not auto-cleared.

2. the strong cache extends the lifetime of the readers by 30-60 seconds to improve sharing as TP instances get recreated over the same base set of DLLs. It gets auto-cleared 30 seconds after last use.

These fixes are still tactical.

The 30 second timeout is just based on an estimate that that is

1. long enough to get sharing of TP instances under changes to a project or re-creation of a TP instance while editing scripts
2. not so long that we have a source of leaks (that is, we assume we don't have an unbounded number of distinct target assembly references pushed through a TP instance within 30 seconds)

[TIHan]

I didn't have a good opinion on this, so I spoke to @heejaechang; this is a valid approach to the problem since there is no other way to handle the cache. In fact, Roslyn has similar thing in some circumstances.

[cartermp]

Approving as this is the tactical fix. We should file issues on this and the F# repo to tie this cache to the lifetime of the hosting FSharpChecker instance.