Replace AgedLookup with Dictionary in XML doc generation and remove dead code

[cartermp]

Another one found by removing the IVT to FSharp.Editor. I don't see why AgedLookup is necessary here, so this replaced it with a Dictionary.

Also removes dead code.

[saul]

Does this need to be a ConcurrentDictionary?

[cartermp]

I don't think so. At least the previous data structure isn't a concurrent one, and AFAIK it's not really a scenario where multiple threads are calling into this routine concurrently.

[KevinRansom]

@cartermp it's probably worth taking a lock, or using concurrent dictionary. We aren't actively requiring ui thread only, so multi threaded use of the IDocumentionBuilder will eventually occur.

[KevinRansom]

@cartermp the aged lookup is a weak reference fix sized cache. Dictionary is not, I assume we believe this won't cause a problem, because the lifetime of this dictionary is quite short.

[cartermp]

I'll look into that

[cartermp]

Note that there doesn't seem to be anything about AgedLookup that helps a multi-threaded scenario, so really the difference here is:

• O(1) lookup time instead of O(n), which AgedLookup has
• Strongly-held data rather than a max of 75 items at a given time

[cartermp]

I think using a concurrent data structure here is probably not the right call at all. The service isn't concurrent as far as I can tell, as this cache exists inside a type that resides inside a ConditionalWeakTable bound to documentationBuilderCache, a module-level value. I think if we were multi-threaded then making this specific cache a concurrent dictionary would be the least of our problems

[forki]

What even is AgedLookup?

[cartermp]

@forki AgedLookup is, as far as I can tell, an older map-like data structure that has an LRU eviction policy and a default of 75 items before it starts evicting. It's also used in the compiler and compiler service.

[forki]

Please add comment in the code so that next time we remember. Maybe even link back to here Phillip Carter <[email protected]> schrieb am Sa., 1. Juni 2019, 18:17:

…

***@***.**** commented on this pull request. ------------------------------ In vsintegration/src/FSharp.Editor/DocComments/XMLDocumentation.fs <#6900 (comment)>: > @@ -215,10 +214,9 @@ module internal XmlDocumentation = /// Provide Xml Documentation type Provider(xmlIndexService:IVsXMLMemberIndexService) = /// Index of assembly name to xml member index. - let mutable xmlCache = new AgedLookup<VsThreadToken,string,IVsXMLMemberIndex>(10,areSimilar=(fun (x,y) -> x = y)) + let cache = Dictionary<string, IVsXMLMemberIndex>() I think using a concurrent data structure here is probably not the right call here at all. The service isn't concurrent as far as I can tell, as this cache exists inside a type that side inside a ConditionalWeakTable bound to documentationBuilderCache, a module-level value. I think if we were multi-threaded then making this specific cache a concurrent dictionary would be the least of our problem — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6900?email_source=notifications&email_token=AAAOANFA4ZLAMQNXZSOQ7QLPYKOKPA5CNFSM4HQX73AKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2KDCPY#discussion_r289610698>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAOANFXGWO3GBYYNOKNJM3PYKOKPANCNFSM4HQX73AA> .

[cartermp]

@forki https://github.com/dotnet/fsharp/blob/master/src/fsharp/InternalCollections.fsi#L5 ?

[dsyme]

Another one found by removing the IVT to FSharp.Editor. I don't see why AgedLookup is necessary here, so this replaced it with a Dictionary.

Why doesn't this change introduce a memory leak? The DIctionary will just grow and grow, won't it, never releasing the IVsXMLMemberIndex?

[cartermp]

Since the object that owns the dictionary resides in a ConditionalWeakTable its lifetime might be a bit different. But if we're still concerned given that then it's easy enough to just add a check for size and clear when it's reached.