search

fsprojects / FSharp.TypeProviders.SDK

The SDK for creating F# type providers
https://fsprojects.github.io/FSharp.TypeProviders.SDK/
MIT License
298 stars 94 forks source link

clear reader cache in background to prevent leak #295

Closed dsyme closed 5 years ago

dsyme commented 5 years ago

@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 commented 5 years ago

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

TIHan commented 5 years ago

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 commented 5 years ago

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 https://github.com/fsprojects/FSharp.TypeProviders.SDK/issues/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 commented 5 years ago

@dsyme see here for issue on the byte[] array: https://github.com/Microsoft/visualfsharp/issues/5931

dsyme commented 5 years ago

@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)