diff --git a/src/VisualStudio/ProjectBase/Automation/VSProject/OAProjectReference.cs b/src/VisualStudio/ProjectBase/Automation/VSProject/OAProjectReference.cs index 9f6578362e..984e6fd594 100644 --- a/src/VisualStudio/ProjectBase/Automation/VSProject/OAProjectReference.cs +++ b/src/VisualStudio/ProjectBase/Automation/VSProject/OAProjectReference.cs @@ -62,7 +62,8 @@ public override EnvDTE.Project SourceProject { get { - if (Guid.Empty == BaseReferenceNode.ReferencedProjectGuid) + var referencedGuid = BaseReferenceNode.ReferencedProjectGuid; + if (Guid.Empty == referencedGuid) { return null; } @@ -70,20 +71,50 @@ public override EnvDTE.Project SourceProject { return null; } + // The ProjectInfo is shared by every project reference that points at this + // project, and it is dropped or cleared when that project closes, unloads or + // reloads. So anything found here is both current and worth reusing, which + // keeps this resolution at once per project instead of once per reference. + // Register the entry when it is missing: the build dependency pass that + // normally creates it can run after this property is first read, and without + // an entry there is nowhere to cache and every caller resolves again. + var projectInfo = ProjectInfo.GetOrCreate(referencedGuid, BaseReferenceNode.Url); + var cached = projectInfo?.DteProject; + if (cached != null) + { + return cached; + } return ThreadHelper.JoinableTaskFactory.Run(async delegate { await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(); - IVsHierarchy hierarchy = VsShellUtilities.GetHierarchy(BaseReferenceNode.ProjectMgr.Site, BaseReferenceNode.ReferencedProjectGuid); - Logger.Information($"OAProjectReference: GetHierarchy for project reference {BaseReferenceNode.ReferencedProjectGuid} returned {(hierarchy != null ? "a hierarchy" : "null")}"); + IVsHierarchy hierarchy = projectInfo?.Hierarchy; + if (hierarchy == null) + { + hierarchy = VsShellUtilities.GetHierarchy(BaseReferenceNode.ProjectMgr.Site, referencedGuid); + Logger.Information($"OAProjectReference: Resolved hierarchy for project reference {referencedGuid} through the shell"); + } + Logger.Information($"OAProjectReference: GetHierarchy for project reference {referencedGuid} returned {(hierarchy != null ? "a hierarchy" : "null")}"); if (null == hierarchy) { return null; } + // Cache the hierarchy even when the automation object below cannot be + // obtained: the hierarchy is what costs a solution wide lookup, and + // leaving it unstored made every later caller pay for it again. + if (projectInfo != null) + { + projectInfo.Hierarchy = hierarchy; + } object extObject; if (Microsoft.VisualStudio.ErrorHandler.Succeeded( - hierarchy.GetProperty(VSConstants.VSITEMID_ROOT, (int)__VSHPROPID.VSHPROPID_ExtObject, out extObject))) + hierarchy.GetProperty(VSConstants.VSITEMID_ROOT, (int)__VSHPROPID.VSHPROPID_ExtObject, out extObject)) + && extObject is EnvDTE.Project project) { - return extObject as EnvDTE.Project; + if (projectInfo != null) + { + projectInfo.DteProject = project; + } + return project; } return null; }); diff --git a/src/VisualStudio/ProjectBase/ProjectInfo.cs b/src/VisualStudio/ProjectBase/ProjectInfo.cs index 3264aa34b7..e47a84b408 100644 --- a/src/VisualStudio/ProjectBase/ProjectInfo.cs +++ b/src/VisualStudio/ProjectBase/ProjectInfo.cs @@ -17,7 +17,50 @@ public class ProjectInfo { public string Url { get; private set; } public Guid Id { get; private set; } - public IVsHierarchy Hierarchy { get; set; } = null; + + private IVsHierarchy _hierarchy = null; + private EnvDTE.Project _dteProject = null; + + /// + /// The hierarchy of this project, once somebody has resolved it. + /// + /// + /// Resolving a hierarchy from a guid enumerates the whole solution, so it is well + /// worth caching. Setting this to null is how close, unload and reload invalidate + /// the entry - see the ClearHierarchy methods. + /// + public IVsHierarchy Hierarchy + { + get { return _hierarchy; } + set + { + _hierarchy = value; + if (value == null) + { + // Invalidation: the automation object was resolved from the hierarchy + // we are dropping, so it has to go too. Replacing it with another non + // null hierarchy is NOT an invalidation: the shell can hand out a + // different runtime wrapper for the very same project, and treating + // that as a change threw the cache away on almost every write. + _dteProject = null; + } + } + } + + /// + /// The automation object of this project, resolved from . + /// + /// + /// Shared by every project reference that points at this project, so the resolution + /// happens once per project instead of once per reference node - on a solution with + /// 226 projects that is 88 resolutions instead of 3283. It is dropped whenever + /// is cleared, so it cannot outlive the project it belongs to. + /// + public EnvDTE.Project DteProject + { + get { return _hierarchy == null ? null : _dteProject; } + set { _dteProject = value; } + } #if DEBUG public string Name => System.IO.Path.GetFileNameWithoutExtension(Url); @@ -64,6 +107,71 @@ public static ProjectInfo GetProjectInfo(string url, Guid guid) return result; } + /// + /// Forget the cached hierarchy of every ProjectInfo that points to it. + /// + /// + /// Readers of take a non null value as proof that the project is + /// still loaded, so it has to be dropped as soon as that project is closed, unloaded or + /// reloaded. Removing the whole ProjectInfo only happens for our own project nodes + /// (ProjectNode.Close()), so foreign projects need this. Clearing too eagerly costs + /// nothing: the next reader resolves the hierarchy through the shell and caches it again. + /// + public static void ClearHierarchy(IVsHierarchy hierarchy) + { + if (hierarchy == null) + { + return; + } + foreach (var projectInfo in _projectsByUrl.Values) + { + if (ReferenceEquals(projectInfo.Hierarchy, hierarchy)) + { + Logger.Information($"Dropping cached hierarchy for {projectInfo.Url} with guid {projectInfo.Id}"); + projectInfo.Hierarchy = null; + } + } + } + + /// + /// Forget all cached hierarchies, for when the whole solution goes away. + /// + public static void ClearHierarchies() + { + Logger.Information("Dropping all cached project hierarchies"); + foreach (var projectInfo in _projectsByUrl.Values) + { + projectInfo.Hierarchy = null; + } + } + + /// + /// Find the entry for a project, registering one when it does not exist yet. + /// + /// + /// ProjectNode.CreateBuildDependencies normally registers these, but it can run + /// after the automation layer has already started reading project references. + /// Anything that wants to cache per project needs an entry to cache on, so it + /// creates one here instead of giving up and resolving again on every call: one + /// session logged 253 solution wide hierarchy lookups for each of 88 projects + /// purely because the entries did not exist yet. + /// Two threads racing here end up with two equivalent entries and the last one + /// wins, which costs at most one extra resolution. + /// + public static ProjectInfo GetOrCreate(Guid id, string url) + { + if (id == Guid.Empty || string.IsNullOrEmpty(url)) + { + return null; + } + var result = GetProjectInfo(url, id); + if (result == null) + { + result = new ProjectInfo(id, url); + } + return result; + } + public static void RemoveProjectInfo(string url, Guid id) { var projectInfo = GetProjectInfo(url, id); diff --git a/src/VisualStudio/ProjectBase/SolutionListenerForProjectReferenceUpdate.cs b/src/VisualStudio/ProjectBase/SolutionListenerForProjectReferenceUpdate.cs index 0c4d35f73b..3a4e872e44 100644 --- a/src/VisualStudio/ProjectBase/SolutionListenerForProjectReferenceUpdate.cs +++ b/src/VisualStudio/ProjectBase/SolutionListenerForProjectReferenceUpdate.cs @@ -44,6 +44,9 @@ public override int OnBeforeCloseProject(IVsHierarchy hierarchy, int removed) ThreadHelper.ThrowIfNotOnUIThread(); Logger.Information($"OnBeforeCloseProject: Project {hierarchy} is being closed. Is it being removed from the solution? {(removed != 0 ? "Yes" : "No")}"); + // This also fires for projects that are not ours, which never run through ProjectNode.Close() + ProjectInfo.ClearHierarchy(hierarchy); + if (removed != 0) { List projectReferences = this.GetProjectReferencesContainingThisProject(hierarchy); @@ -71,6 +74,9 @@ public override int OnAfterLoadProject(IVsHierarchy stubHierarchy, IVsHierarchy { ThreadHelper.ThrowIfNotOnUIThread(); + // The stub is replaced by realHierarchy, so anything cached for it is stale now + ProjectInfo.ClearHierarchy(stubHierarchy); + List projectReferences = this.GetProjectReferencesContainingThisProject(realHierarchy); Logger.Information($"OnAfterLoadProject:Project {realHierarchy} is being loaded. Updating {projectReferences.Count} project references that point to it."); @@ -145,10 +151,21 @@ public override int OnAfterRenameProject(IVsHierarchy hierarchy) } + public override int OnAfterCloseSolution(object reserved) + { + // Nothing that was cached while the solution was open can be trusted anymore + ProjectInfo.ClearHierarchies(); + return VSConstants.S_OK; + } + + public override int OnBeforeUnloadProject(IVsHierarchy realHierarchy, IVsHierarchy stubHierarchy) { ThreadHelper.ThrowIfNotOnUIThread(); + // realHierarchy is going away, and this also fires for projects that are not ours + ProjectInfo.ClearHierarchy(realHierarchy); + List projectReferences = this.GetProjectReferencesContainingThisProject(realHierarchy); Logger.Information($"OnBeforeUnloadProject: Project {realHierarchy} is being unloaded. Updating {projectReferences.Count} project references that point to it."); diff --git a/src/VisualStudio/XSharpCodeModelXs/Database/XDatabase.prg b/src/VisualStudio/XSharpCodeModelXs/Database/XDatabase.prg index 4aea94747f..5487d19367 100644 --- a/src/VisualStudio/XSharpCodeModelXs/Database/XDatabase.prg +++ b/src/VisualStudio/XSharpCodeModelXs/Database/XDatabase.prg @@ -24,6 +24,7 @@ STATIC CLASS XDatabase STATIC PRIVATE oConn AS DbConnection // In memory database ! STATIC PRIVATE lastWritten := DateTime.MinValue AS DateTime + STATIC PRIVATE backupRunning := 0 AS LONG // guards against overlapping backups STATIC PRIVATE currentFile AS STRING STATIC PROPERTY FileName as STRING GET currentFile STATIC PROPERTY DeleteOnClose as LOGIC AUTO @@ -173,12 +174,17 @@ STATIC METHOD SafeFileDelete(cFile as STRING) AS VOID var deleted := false do while tries < 4 .and. !deleted try - System.Threading.Thread.Sleep(tries * 100) File.Delete(cFile) deleted := true catch as IOException Log(i"Failed to delete file {cFile}, attempts {tries}") tries++ + // Only back off when we are actually going to try again. This used to + // sleep before the first attempt as well, which cost 100 ms on every + // single backup for nothing. + if tries < 4 + System.Threading.Thread.Sleep(tries * 100) + endif end try enddo if ! deleted @@ -188,11 +194,18 @@ STATIC METHOD SafeFileDelete(cFile as STRING) AS VOID STATIC METHOD SaveToDisk(oConn AS DbConnection, cFile AS STRING) AS VOID CHECKIFOPEN + // Only the BackupDatabase call below touches the in memory connection. Deleting + // the old file, opening the disk database and vacuuming it all work on the disk + // side, so they must stay outside the lock on oConn: that lock serializes the + // whole code model, and holding it for a full backup plus a VACUUM blocked every + // parse and every lookup for seconds at a time. Log(i"SafeDelete file {cFile}") SafeFileDelete(cFile) USING VAR diskdb := OpenFile(cFile) Log(i"Save DB to disk {cFile}") - oConn:BackupDatabase(diskdb, "main") + BEGIN LOCK oConn + oConn:BackupDatabase(diskdb, "main") + END LOCK USING VAR oCmd := CreateCommand("VACUUM", diskdb) Log(i"Execute VACUUM command") oCmd:ExecuteNonQuery() @@ -211,27 +224,39 @@ STATIC METHOD CommitWhenNeeded() AS VOID VAR ts := DateTime.Now - lastWritten // Save to disk every 5 minutes Log(i"Time since last backup {ts}") - IF ts:Minutes >= 5 .OR. ts:Hours > 0 - LOCAL oBW AS BackgroundWorker - oBW := BackgroundWorker{} - oBW:DoWork += BackupInBackground - oBW:RunWorkerAsync() - + IF ts:TotalMinutes < 5 + RETURN ENDIF + // Let one backup run at a time. This is called from every write to the database, + // so without the guard each caller inside the 5 minute window starts its own + // BackgroundWorker and they all queue up behind each other: the logs show runs of + // 5 and 6 backups back to back, 32 seconds of the code model being unavailable. + IF System.Threading.Interlocked.CompareExchange(REF backupRunning, 1, 0) != 0 + Log("A backup is already running, skipping this one") + RETURN + ENDIF + // Claim the interval right away. SaveToDisk sets it again when it has finished, + // but until then everybody else must already see this interval as taken care of. + lastWritten := DateTime.Now + LOCAL oBW AS BackgroundWorker + oBW := BackgroundWorker{} + oBW:DoWork += BackupInBackground + oBW:RunWorkerAsync() STATIC METHOD BackupInBackground(sender AS OBJECT , args AS DoWorkEventArgs ) AS VOID - CHECKIFOPEN - BEGIN LOCK oConn - TRY - Log(i"Starting backup to {currentFile}") - SaveToDisk(oConn, currentFile ) - CATCH e AS Exception - Log(i"Error backing up to {currentFile}") - XSettings.Exception(e) - FINALLY - Log(i"Completed backup to {currentFile}") - END TRY - END LOCK + TRY + CHECKIFOPEN + Log(i"Starting backup to {currentFile}") + // SaveToDisk takes the lock on oConn itself, and only around the part that + // actually needs it. + SaveToDisk(oConn, currentFile ) + CATCH e AS Exception + Log(i"Error backing up to {currentFile}") + XSettings.Exception(e) + FINALLY + Log(i"Completed backup to {currentFile}") + System.Threading.Interlocked.Exchange(REF backupRunning, 0) + END TRY RETURN STATIC METHOD CreateSchema(Connection AS DbConnection) AS VOID @@ -602,6 +627,33 @@ STATIC METHOD ValidateSchema( Connection AS DbConnection) AS LOGIC Log(i"Validate database schema: {lOk}") RETURN lOk +STATIC PRIVATE METHOD ExecuteSimpleSql(cSql AS STRING) AS VOID + // For statements that take no parameters and return nothing, such as BEGIN and COMMIT. + // The caller is expected to hold the lock on oConn. + USING VAR cmd := CreateCommand(cSql, oConn) + cmd:ExecuteNonQuery() + RETURN + +STATIC METHOD DeleteOrphanIncludeFiles() AS VOID + // Drop IncludeFiles rows that no longer belong to any file. + // This used to run inside UpdateFileContents, once for every file written: a full anti + // join over IncludeFilesPerFile, while holding the lock that serializes the whole code + // model. A cold walk of RadixWf.sln does that 39387 times. Orphan rows are harmless + // until they are cleaned up - nothing reads them, and UpdateIncludeFiles reuses a row + // when the include comes back - so once per project walk is enough. + CHECKIFOPEN + BEGIN LOCK oConn + TRY + Log("Delete orphan include files") + USING VAR cmd := CreateCommand("Delete from IncludeFiles where Id not in (select IdInclude from IncludeFilesPerFile)", oConn) + cmd:ExecuteNonQuery() + CATCH e AS Exception + Log("Error deleting orphaned include files") + XSettings.Exception(e) + END TRY + END LOCK + RETURN + STATIC METHOD DeleteOrphanFiles() AS List VAR result := List{} CHECKIFOPEN result @@ -1324,8 +1376,17 @@ STATIC PRIVATE METHOD UpdateFileContents(oFile AS XFile) AS VOID NEXT NEXT Log(i"Start Updating File contents for file {oFile.FullPath} : # of Entities {oFile.EntityList.Count}") + LOCAL lInTransaction := FALSE AS LOGIC BEGIN LOCK oConn TRY + // One transaction for the whole file. Without it every statement below is its + // own implicit transaction, and AddTypes/AddMembers issue one INSERT per type + // and per member: a file with 159 entities costs about 160 of them. + // BEGIN/COMMIT are sent as plain SQL on purpose. Handing out a DbTransaction + // would mean assigning it to every command created further down the call chain, + // and Microsoft.Data.Sqlite throws when a command misses it. + ExecuteSimpleSql("BEGIN IMMEDIATE") + lInTransaction := TRUE // Check to see if file is in multiple projects. // If so then generate a new type for each of the projects @@ -1362,15 +1423,26 @@ STATIC PRIVATE METHOD UpdateFileContents(oFile AS XFile) AS VOID // Update Includefile IDs and write to disk UpdateIncludeFiles(oFile) endif - // Remove orphans from IncludeFiles table - oCmd:CommandText := "Delete from IncludeFiles where Id not in (select IdInclude from IncludeFilesPerFile)" - oCmd:Parameters:Clear() - oCmd:ExecuteScalar() + // Orphans in the IncludeFiles table are collected once per project walk, see + // DeleteOrphanIncludeFiles(). Doing it here meant a full anti join per file. + + ExecuteSimpleSql("COMMIT") + lInTransaction := FALSE CATCH e AS Exception Log("File : "+oFile:FullPath+" "+oFile:Id:ToString()) XSettings.Exception(e) + FINALLY + IF lInTransaction + // The commit never happened. Undo the half written file and, more + // importantly, leave the connection out of a transaction for the next one. + TRY + ExecuteSimpleSql("ROLLBACK") + CATCH + NOP + END TRY + ENDIF END TRY END LOCK diff --git a/src/VisualStudio/XSharpCodeModelXs/Parser/ModelWalker.prg b/src/VisualStudio/XSharpCodeModelXs/Parser/ModelWalker.prg index 20d5dbea45..1b2b14380c 100644 --- a/src/VisualStudio/XSharpCodeModelXs/Parser/ModelWalker.prg +++ b/src/VisualStudio/XSharpCodeModelXs/Parser/ModelWalker.prg @@ -220,6 +220,10 @@ PRIVATE STATIC METHOD WalkSource() AS VOID XDatabase.DeleteFile(fileName) endif next + // Collect the include file orphans left behind by this walk. This used to run + // once per file inside the database lock, which is far too often for a full + // anti join over the whole table. + XDatabase.DeleteOrphanIncludeFiles() if _currentProject != null _currentProject:ProjectWalkComplete?:Invoke(_currentProject) endif diff --git a/src/VisualStudio/XSharpCodeModelXs/ProjectSystem/XFile.prg b/src/VisualStudio/XSharpCodeModelXs/ProjectSystem/XFile.prg index 414d5352b9..d6def593b8 100644 --- a/src/VisualStudio/XSharpCodeModelXs/ProjectSystem/XFile.prg +++ b/src/VisualStudio/XSharpCodeModelXs/ProjectSystem/XFile.prg @@ -166,7 +166,9 @@ NAMESPACE XSharpModel IF ! SELF:Virtual XDatabase.Update(SELF) SELF:Project:ClearCache(SELF) - SELF:Project:RefreshGlobalUsings() + // Just mark them stale. This used to rebuild the project's global usings here, + // which is a database query, for every single file written. + SELF:Project:InvalidateGlobalUsings() IF ! SELF:Interactive SELF:Clear() ENDIF diff --git a/src/VisualStudio/XSharpCodeModelXs/ProjectSystem/XProject.prg b/src/VisualStudio/XSharpCodeModelXs/ProjectSystem/XProject.prg index b067b6affb..53a4f0822a 100644 --- a/src/VisualStudio/XSharpCodeModelXs/ProjectSystem/XProject.prg +++ b/src/VisualStudio/XSharpCodeModelXs/ProjectSystem/XProject.prg @@ -1,4 +1,4 @@ -// +// // Copyright (c) XSharp B.V. All Rights Reserved. // Licensed under the Apache License, Version 2.0. // See License.txt in the project root for license information. @@ -46,6 +46,11 @@ CLASS XProject PRIVATE _resolvingReferences AS LOGIC private _globalUsings AS List private _globalStaticUsing AS List + // Set when a file is written to the database, cleared when the lists are rebuilt. + // Rebuilding queries the database, so it must not happen once per saved file. + // Starts TRUE so the first reader fills the lists even when the project is opened + // from an up to date database and no file is ever written. + private _globalUsingsDirty := TRUE AS LOGIC PRIVATE _cachedAllNamespaces AS IList PRIVATE _cachedUsingStatics AS IList @@ -60,8 +65,8 @@ CLASS XProject PROPERTY HasFiles AS LOGIC GET _SourceFilesDict:Keys:Count > 0 .or. _OtherFilesDict:Keys:Count > 0 PROPERTY Framework AS STRING GET _framework PROPERTY DisplayName AS STRING GET _projectNode?.DisplayName - property GlobalUsings AS List GET SELF:_globalUsings - property GlobalStaticUsings AS List GET SELF:_globalStaticUsing + property GlobalUsings AS List GET SELF:EnsureGlobalUsings():_globalUsings + property GlobalStaticUsings AS List GET SELF:EnsureGlobalUsings():_globalStaticUsing PROPERTY DependentAssemblyList AS STRING GET @@ -656,22 +661,40 @@ CLASS XProject #endregion #region 'Normal' Files + // Mark the global usings as out of date. Deliberately cheap: this is called for every + // file that gets written to the database, and rebuilding the lists means a query, so + // the work is deferred until somebody actually reads them. + METHOD InvalidateGlobalUsings() AS VOID + SELF:_globalUsingsDirty := TRUE + + // Rebuild the lists when they are stale. Returns SELF so the properties can chain. + PRIVATE METHOD EnsureGlobalUsings() AS XProject + IF SELF:_globalUsingsDirty + SELF:RefreshGlobalUsings() + ENDIF + RETURN SELF + METHOD RefreshGlobalUsings() AS VOID + SELF:_globalUsingsDirty := FALSE var usings := XDatabase.GetProjectGlobalUsings(SELF:Id) - SELF:_globalUsings:Clear() - SELF:_globalStaticUsing:Clear() + // Build into fresh lists and swap them in, so a reader iterating the old list never + // sees it half emptied. The walker writes from several threads at once. + var newUsings := List{} + var newStatics := List{} foreach var item in usings if item:Attributes:HasFlag(Modifiers.Global) if item:Attributes:HasFlag(Modifiers.Static) - SELF:AddUniqueUsing(_globalStaticUsing, item:Namespace) + SELF:AddUniqueUsing(newStatics, item:Namespace) else - SELF:AddUniqueUsing(_globalUsings, item:Namespace) + SELF:AddUniqueUsing(newUsings, item:Namespace) endif endif next - SELF:AddUniqueUsing(_globalUsings, "System") - SELF:AddUniqueUsing(_globalUsings, "XSharp") + SELF:AddUniqueUsing(newUsings, "System") + SELF:AddUniqueUsing(newUsings, "XSharp") + SELF:_globalUsings := newUsings + SELF:_globalStaticUsing := newStatics METHOD AddUniqueUsing(list as List, name as string) AS VOID var old := list:Find( { x => x:ToUpper() == name:ToUpper()}) if String.IsNullOrEmpty(old) @@ -1577,7 +1600,7 @@ CLASS XProject NEXT result := asmNS ENDIF - FOREACH var ns in SELF:_globalUsings + FOREACH var ns in SELF:GlobalUsings if !result:Contains(ns) result:Add(ns) endif @@ -1601,7 +1624,7 @@ CLASS XProject ENDIF NEXT ENDIF - FOREACH var ns in SELF:_globalStaticUsing + FOREACH var ns in SELF:GlobalStaticUsings if !statics:Contains(ns) statics:Add(ns) endif