Skip to content
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.

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

[WIP] Improve solution load performance #2909

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,10 @@
<HintPath>$(FSharpSourcesRoot)\..\packages\Microsoft.VisualStudio.Shell.Interop.11.0.11.0.61030\lib\Microsoft.VisualStudio.Shell.Interop.11.0.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Microsoft.VisualStudio.Shell.Interop.12.0">
<HintPath>$(FSharpSourcesRoot)\..\packages\Microsoft.VisualStudio.Shell.Interop.12.0.12.0.30110\lib\Microsoft.VisualStudio.Shell.Interop.12.0.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="Microsoft.VisualStudio.TextManager.Interop">
<HintPath>$(FSharpSourcesRoot)\..\packages\Microsoft.VisualStudio.TextManager.Interop.7.10.6070\lib\Microsoft.VisualStudio.TextManager.Interop.dll</HintPath>
<Private>True</Private>
Expand Down
263 changes: 173 additions & 90 deletions vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

Large diffs are not rendered by default.

50 changes: 36 additions & 14 deletions vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ open Microsoft.VisualStudio
open Microsoft.VisualStudio.TextManager.Interop
open Microsoft.VisualStudio.Shell.Interop
open Microsoft.FSharp.Compiler.SourceCodeServices
open System.Collections.Concurrent
open System.Globalization

type internal CacheUsage =
| UseCache
| IgnoreCache

/// An additional interface that an IProjectSite object can implement to indicate it has an FSharpProjectOptions
/// already available, so we don't have to recreate it
Expand Down Expand Up @@ -121,26 +127,41 @@ type internal ProjectSitesAndFiles() =
| _ -> None)
| None -> Seq.empty

static let projectCache = ConcurrentDictionary<string,FSharpProjectOptions>()
Copy link
Contributor

Choose a reason for hiding this comment

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

What removes entries from this cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing yet - good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


static let rec referencedProjectsOf (projectSite:IProjectSite, fileName, extraProjectInfo, serviceProvider:System.IServiceProvider) =
referencedProvideProjectSites (projectSite, serviceProvider)
|> Seq.choose (fun (p, ps) ->
fullOutputAssemblyPath p
|> Option.map (fun path ->
path, getProjectOptionsForProjectSite (ps.GetProjectSite(), fileName, extraProjectInfo, serviceProvider))
path, getProjectOptionsForProjectSite (ps.GetProjectSite(), fileName, extraProjectInfo, serviceProvider, UseCache))
)
|> Seq.toArray

and getProjectOptionsForProjectSite(projectSite:IProjectSite, fileName, extraProjectInfo, serviceProvider) =
{ProjectFileName = projectSite.ProjectFileName()
ProjectFileNames = projectSite.SourceFilesOnDisk()
OtherOptions = projectSite.CompilerFlags()
ReferencedProjects = referencedProjectsOf(projectSite, fileName, extraProjectInfo, serviceProvider)
IsIncompleteTypeCheckEnvironment = projectSite.IsIncompleteTypeCheckEnvironment
UseScriptResolutionRules = SourceFile.MustBeSingleFileProject fileName
LoadTime = projectSite.LoadTime
UnresolvedReferences = None
OriginalLoadReferences = []
ExtraProjectInfo=extraProjectInfo }
and getProjectOptionsForProjectSite(projectSite:IProjectSite, fileName:string, extraProjectInfo, serviceProvider, cacheUsage:CacheUsage) =
Copy link
Contributor

Choose a reason for hiding this comment

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

This will clash with my other proposed fix. The project options are already stored in a table in LanguageService.fs, it seems to me we should just callback to there to look up the table.

let cacheKey = fileName.ToLower CultureInfo.CurrentUICulture
match cacheUsage, projectCache.TryGetValue cacheKey with
| UseCache, (true, options) ->
{ options with ExtraProjectInfo = extraProjectInfo }
| _ ->
let options =
{
ProjectFileName = projectSite.ProjectFileName()
ProjectFileNames = projectSite.SourceFilesOnDisk()
OtherOptions = projectSite.CompilerFlags()
ReferencedProjects = referencedProjectsOf(projectSite, fileName, extraProjectInfo, serviceProvider)
IsIncompleteTypeCheckEnvironment = projectSite.IsIncompleteTypeCheckEnvironment
UseScriptResolutionRules = SourceFile.MustBeSingleFileProject fileName
LoadTime = projectSite.LoadTime
UnresolvedReferences = None
OriginalLoadReferences = []
ExtraProjectInfo = extraProjectInfo
}
projectCache.[cacheKey] <- options
options

static member ClearCache() : unit =
projectCache.Clear()

/// Construct a project site for a single file. May be a single file project (for scripts) or an orphan project site (for everything else).
static member ProjectSiteOfSingleFile(filename:string) : IProjectSite =
Expand Down Expand Up @@ -227,9 +248,10 @@ type internal ProjectSitesAndFiles() =
/// Create project options for this project site.
static member GetProjectOptionsForProjectSite(projectSite:IProjectSite,filename,extraProjectInfo,serviceProvider:System.IServiceProvider) =
match projectSite with
| :? IHaveCheckOptions as hco -> hco.OriginalCheckOptions()
| :? IHaveCheckOptions as hco ->
hco.OriginalCheckOptions()
| _ ->
getProjectOptionsForProjectSite(projectSite, filename, extraProjectInfo, serviceProvider)
getProjectOptionsForProjectSite(projectSite, filename, extraProjectInfo, serviceProvider, IgnoreCache)

/// Create project site for these project options
static member CreateProjectSiteForScript (filename, checkOptions) = ProjectSiteOfScriptFile (filename, checkOptions) :> IProjectSite
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public override string Url
{
get
{
return this.myAssemblyPath;
return Path.GetFullPath(this.myAssemblyPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

This fix still looks useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was used by the caching code to determine if two references were the same. this.myAssemblyPath is not absolute and so it made comparing them difficult when the rooted-ness was inconsistent within a solution. It's not a bug fix per se.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ public override string Url

public void SetDebugLogger(Microsoft.Build.Framework.ILogger debugLogger)
{
myDebugLogger = debugLogger;
//myDebugLogger = debugLogger;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporary change, should not have committed

}


Expand Down Expand Up @@ -3128,7 +3128,9 @@ internal virtual void SetBuildConfigurationProperties(ConfigCanonicalName config
}
}

public abstract void NotifySourcesAndFlags();
public abstract void ComputeSourcesAndFlags();
public abstract IDisposable AcquireExclusiveBuildResource(bool actuallyBuild);

internal abstract int FixupAppConfigOnTargetFXChange(string newTargetFramework, string targetFSharpCoreVersion, bool autoGenerateBindingRedirects);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ private string GetReferencedProjectOutputPath()
// build the full path adding the name of the assembly to the output path.
outputPath = System.IO.Path.Combine(outputPath, assemblyName);

return outputPath;
return System.IO.Path.GetFullPath(outputPath);

}

Expand Down Expand Up @@ -872,11 +872,12 @@ private static System.Runtime.Versioning.FrameworkName GetProjectTargetFramework
if (hierarchy == null)
return null;

object otherTargetFrameworkMonikerObj;

hierarchy.GetProperty(VSConstants.VSITEMID_ROOT, (int)__VSHPROPID4.VSHPROPID_TargetFrameworkMoniker, out otherTargetFrameworkMonikerObj);

string targetFrameworkMoniker = (string)otherTargetFrameworkMonikerObj;
object otargetFrameworkMoniker = null;
int hr = hierarchy.GetProperty(VSConstants.VSITEMID_ROOT, (int)__VSHPROPID4.VSHPROPID_TargetFrameworkMoniker, out otargetFrameworkMoniker);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correctness fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this stops an occasional assertion on startup

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thanks

if (!ErrorHandler.Succeeded(hr))
return null;

string targetFrameworkMoniker = (string)otargetFrameworkMoniker;
return new System.Runtime.Versioning.FrameworkName(targetFrameworkMoniker);
}

Expand Down
70 changes: 60 additions & 10 deletions vsintegration/src/FSharp.ProjectSystem.FSharp/Project.fs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
type internal FSharpProjectPackage() as this =
inherit ProjectPackage()

let mutable flagsService = Unchecked.defaultof<ProjectFlagsService>

let mutable vfsiToolWindow = Unchecked.defaultof<Microsoft.VisualStudio.FSharp.Interactive.FsiToolWindow>
let GetToolWindowAsITestVFSI() =
if vfsiToolWindow = Unchecked.defaultof<_> then
Expand All @@ -199,6 +201,8 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem

let thisLock = obj()

member this.FlagsService = flagsService

member this.RegisterForIdleTime() =

if not (isNull mgr) && componentID = 0u then
Expand Down Expand Up @@ -312,6 +316,13 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
//TODO the TypeProviderSecurityGlobals does not exists anymore, remove the initialization?
this.GetService(typeof<FSharpLanguageService>) |> ignore

//let flagsService = this.GetService(typeof<ProjectFlagsService>) :?> ProjectFlagsService
//flagsService.Initialize()

let accessor = this.GetService(typeof<SVsBuildManagerAccessor>) :?> IVsBuildManagerAccessor
flagsService <- ProjectFlagsService(accessor)
flagsService.Initialize()

// FSI-LINKAGE-POINT: sited init
let commandService = this.GetService(typeof<IMenuCommandService>) :?> OleMenuCommandService // FSI-LINKAGE-POINT
Microsoft.VisualStudio.FSharp.Interactive.Hooks.fsiConsoleWindowPackageInitalizeSited (this :> Package) commandService
Expand Down Expand Up @@ -473,7 +484,7 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
#if DEBUG
let uiThreadId = System.Threading.Thread.CurrentThread.ManagedThreadId
let mutable compileWasActuallyCalled = false
#endif
#endif

// these get initialized once and for all in SetSite()
let mutable isInCommandLineMode = false
Expand All @@ -491,7 +502,7 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
this.ImageHandler.AddImage(FSharpSR.GetObject("4100") :?> System.Drawing.Bitmap) // 4008 = EmptyProject
this.ImageHandler.AddImage(FSharpSR.GetObject("4103") :?> System.Drawing.Bitmap) // 4006 = ScriptFile
this.ImageHandler.AddImage(FSharpSR.GetObject("4102") :?> System.Drawing.Bitmap) // 4007 = Signature

/// Provide mapping from our browse objects and automation objects to our CATIDs
do
// The following properties classes are specific to F# so we can use their GUIDs directly
Expand Down Expand Up @@ -1346,21 +1357,60 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
else
0

// returns an array of all "foo"s of form: <Compile Include="foo"/>
/// Returns an array of all files specified for compile in the build project.
/// Returns all: <Compile Include="foo" />
member private x.ComputeCompileItems() =
FSharpProjectNode.ComputeCompileItems(x.BuildProject, x.ProjectFolder)

static member ComputeCompileItems(buildProject, projectFolder) =
[|
for i in buildProject.Items do
if i.ItemType = "Compile" then
yield System.IO.Path.GetFullPath(System.IO.Path.Combine(projectFolder, i.EvaluatedInclude))
|]
member x.GetCompileItems() = let sources,_ = sourcesAndFlags.Value in sources
member x.GetCompileFlags() = let _,flags = sourcesAndFlags.Value in flags

/// Rudimentary approximation of compile flags
member private x.ComputeCompileFlags() =
[|
for ref in x.GetReferenceContainer().EnumReferences() do
match ref with
| :? AssemblyReferenceNode ->
yield sprintf "-r:%s" ref.Url
| :? ProjectReferenceNode as projRef ->
yield sprintf "-r:%s" projRef.ReferencedProjectOutputPath
| _ -> ()
yield "--noframework"
|]

member x.GetCompileItems() =
let sources, _ = sourcesAndFlags.Value in sources

member x.GetCompileFlags() =
let _, flags = sourcesAndFlags.Value in flags

override this.AcquireExclusiveBuildResource(newActuallyBuild) =
let oldActuallyBuild = actuallyBuild
actuallyBuild <- newActuallyBuild

{ new IDisposable with
override __.Dispose() =
actuallyBuild <- oldActuallyBuild
}

override this.NotifySourcesAndFlags() =
sourcesAndFlagsNotifier.Notify()

override this.ComputeSourcesAndFlags() =

if not this.IsInBatchUpdate && box this.BuildProject <> null && not inMidstOfReloading then

// TODO: estimate what our current flags/sources are

override x.ComputeSourcesAndFlags() =
//let flagsService : ProjectFlagsService = GetService(this.Site)
package.FlagsService.QueueProject this

if x.IsInBatchUpdate || box x.BuildProject = null then ()
(*
if true || x.IsInBatchUpdate || box x.BuildProject = null then ()
else
if not(inMidstOfReloading) && not(VsBuildManagerAccessorExtensionMethods.IsInProgress(accessor)) then

Expand Down Expand Up @@ -1402,9 +1452,9 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
// taken from System.Runtime that is not supplied

let _ = x.InvokeMsBuild("Compile", extraProperties = [KeyValuePair("_ResolveReferenceDependencies", "true")])
sourcesAndFlagsNotifier.Notify()
finally
actuallyBuild <- true
*)

member internal x.DetermineRuntimeAndSKU(targetFrameworkMoniker : string) =
let frameworkName = new System.Runtime.Versioning.FrameworkName(targetFrameworkMoniker)
Expand Down Expand Up @@ -1558,7 +1608,7 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
Debug.Assert(System.Threading.Thread.CurrentThread.ManagedThreadId = uiThreadId, "called GetProjectSite() while still in Opening state from non-UI thread")
#endif
x.ComputeSourcesAndFlags()
if not(projectSite.State = ProjectSiteOptionLifetimeState.Opened) then
if projectSite.State <> ProjectSiteOptionLifetimeState.Opened then
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 :)

// We failed to Build. This can happen e.g. when the user has custom MSBuild functionality under the "Compile" target, e.g. a CompileDependsOn that fails.
// We now have no reliable way to get information about the project.
// Rather than be in a completely useless state, we just report 0 source files and 0 compiler flags.
Expand All @@ -1567,7 +1617,7 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
// Once that error is fixed, a future call to ComputeSourcesAndFlags() will successfully call through to our HostObject and get to Compile(),
// which will finally populate sourcesAndFlags with good values.
// This means that ones the user fixes the problem, proper intellisense etc. should start immediately lighting up.
sourcesAndFlags <- Some([||],[||])
sourcesAndFlags <- Some(x.ComputeCompileItems(), [||]) // x.ComputeCompileFlags())
projectSite.Open(x.CreateRunningProjectSite())
()
| _ -> ()
Expand Down
Loading