Skip to content

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

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
KevinRansom merged 2 commits into from
Jun 2, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa931ee
Replace AgedLookup with Dictionary in XML doc generation
cartermp May 29, 2019
a46ddc2
Try remove unecessary stuff
cartermp Jun 1, 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
Replace AgedLookup with Dictionary in XML doc generation
  • Loading branch information
@cartermp
cartermp committed May 29, 2019
commit aa931eea001eb46f5dfc45941912b29d20d65e6f
18 changes: 8 additions & 10 deletions vsintegration/src/FSharp.Editor/DocComments/XMLDocumentation.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ namespace Microsoft.VisualStudio.FSharp.Editor

open System
open System.Runtime.CompilerServices
open System.Runtime.Caching
open System.Text.RegularExpressions
open Internal.Utilities.Collections
open Microsoft.VisualStudio.Shell
open Microsoft.VisualStudio.Shell.Interop
open FSharp.Compiler.SourceCodeServices
open FSharp.Compiler.Layout
open FSharp.Compiler.Layout.TaggedTextOps
open System.Collections.Generic

type internal ITaggedTextCollector =
abstract Add: text: TaggedText -> unit
Expand Down Expand Up @@ -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>()
Copy link
Contributor

@saul saul May 30, 2019

Choose a reason for hiding this comment

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

Does this need to be a ConcurrentDictionary?

Copy link
Contributor Author

@cartermp cartermp May 30, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@KevinRansom KevinRansom May 31, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@KevinRansom KevinRansom May 31, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cartermp cartermp May 31, 2019

Choose a reason for hiding this comment

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

I'll look into that

Copy link
Contributor Author

@cartermp cartermp Jun 1, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cartermp cartermp Jun 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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


do Events.SolutionEvents.OnAfterCloseSolution.Add (fun _ ->
xmlCache.Clear(vsToken))
do Events.SolutionEvents.OnAfterCloseSolution.Add (fun _ -> cache.Clear())

#if DEBUG // Keep under DEBUG so that it can keep building.

Expand Down Expand Up @@ -251,14 +249,14 @@ module internal XmlDocumentation =

/// Retrieve the pre-existing xml index or None
let GetMemberIndexOfAssembly(assemblyName) =
match xmlCache.TryGet(vsToken, assemblyName) with
| Some(memberIndex) -> Some(memberIndex)
| None ->
match cache.TryGetValue(assemblyName) with
| true, memberIndex -> Some(memberIndex)
| false, _ ->
let ok,memberIndex = xmlIndexService.CreateXMLMemberIndex(assemblyName)
if Com.Succeeded(ok) then
let ok = memberIndex.BuildMemberIndex()
if Com.Succeeded(ok) then
xmlCache.Put(vsToken, assemblyName,memberIndex)
if Com.Succeeded(ok) then
cache.Add(assemblyName, memberIndex)
Some(memberIndex)
else None
else None
Expand Down