From 11e245f9c146cc3d998f625ace49a4efe9c4164d Mon Sep 17 00:00:00 2001 From: Paul van Brenk Date: Tue, 17 Apr 2018 15:57:19 -0700 Subject: [PATCH 1/5] Make HierarchyId map easier to understand --- .../Nodejs/SharedProject/HierarchyIdMap.cs | 75 +++++++------------ 1 file changed, 28 insertions(+), 47 deletions(-) diff --git a/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs b/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs index 5a4a26553..30f4f6c20 100644 --- a/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs +++ b/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs @@ -1,25 +1,25 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; +using System.Collections.Concurrent; using System.Diagnostics; namespace Microsoft.VisualStudioTools.Project { internal sealed class HierarchyIdMap { - private readonly List> ids = new List>(); - private readonly Stack freedIds = new Stack(); - - private readonly object theLock = new object(); + private readonly ConcurrentDictionary> nodes = new ConcurrentDictionary>(); + private readonly ConcurrentStack freedIds = new ConcurrentStack(); /// /// Must be called from the UI thread /// public uint Add(HierarchyNode node) { + VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); + #if DEBUG - foreach (var reference in this.ids) + foreach (var reference in this.nodes.Values) { if (reference != null) { @@ -29,25 +29,15 @@ public uint Add(HierarchyNode node) } } } -#endif - VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); - - lock (this.theLock) +#endif + if (!this.freedIds.TryPop(out var idx)) { - if (this.freedIds.Count > 0) - { - var i = this.freedIds.Pop(); - this.ids[i] = new WeakReference(node); - return (uint)i + 1; - } - else - { - this.ids.Add(new WeakReference(node)); - // ids are 1 based - return (uint)this.ids.Count; - } + idx = this.nodes.Count; } + this.nodes[idx] = new WeakReference(node); + + return (uint)idx + 1; } /// @@ -57,33 +47,21 @@ public void Remove(HierarchyNode node) { VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); - if (node == null) - { - throw new ArgumentNullException(nameof(node)); - } + Debug.Assert(node != null, "Called with null node"); - lock (this.theLock) - { - var i = (int)node.ID - 1; - if (0 > i || i >= this.ids.Count) - { - throw new InvalidOperationException($"Invalid id. {i}"); - } + var idx = (int)node.ID - 1; - var weakRef = this.ids[i]; - if (weakRef == null) - { - throw new InvalidOperationException("Trying to retrieve a node before adding."); - } + var removeCheck = this.nodes.TryGetValue(idx, out var weakRef); - if (weakRef.TryGetTarget(out var found) && !object.ReferenceEquals(node, found)) - { - throw new InvalidOperationException("The node has the wrong id."); - } + Debug.Assert(removeCheck, "How did we get an id, which we haven't seen before"); + Debug.Assert(weakRef != null, "Double delete is not expected."); - this.ids[i] = null; - this.freedIds.Push(i); - } + var checkReference = weakRef.TryGetTarget(out var found) && object.ReferenceEquals(node, found); + + Debug.Assert(checkReference, "The node has the wrong id."); + + this.nodes[idx] = null; + this.freedIds.Push(idx); } /// @@ -93,12 +71,15 @@ public HierarchyNode this[uint itemId] { get { + VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); + var i = (int)itemId - 1; - if (0 <= i && i < this.ids.Count) + if (0 <= i && i < this.nodes.Count) { - var reference = this.ids[i]; + var reference = this.nodes[i]; if (reference != null && reference.TryGetTarget(out var node)) { + Debug.Assert(node != null); return node; } } From f50529f5b6d16ee7f45261e935ad5fc19bdfbfa9 Mon Sep 17 00:00:00 2001 From: Paul van Brenk Date: Tue, 17 Apr 2018 15:57:39 -0700 Subject: [PATCH 2/5] Ensure when we add a folder we use a trailing slash --- Common/Product/SharedProject/CommonUtils.cs | 3 ++- .../CommonProjectNode.DiskMerger.cs | 18 ++++++------------ .../CommonProjectNode.FileSystemChange.cs | 2 +- .../Nodejs/SharedProject/CommonProjectNode.cs | 6 ++++-- 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/Common/Product/SharedProject/CommonUtils.cs b/Common/Product/SharedProject/CommonUtils.cs index 918caba18..c6ada0d3b 100644 --- a/Common/Product/SharedProject/CommonUtils.cs +++ b/Common/Product/SharedProject/CommonUtils.cs @@ -550,7 +550,8 @@ public static string EnsureEndSeparator(string path) { if (string.IsNullOrEmpty(path)) { - return string.Empty; + Debug.Fail("what??"); + return path; } else if (!HasEndSeparator(path)) { diff --git a/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.DiskMerger.cs b/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.DiskMerger.cs index 870140862..39e6d12af 100644 --- a/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.DiskMerger.cs +++ b/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.DiskMerger.cs @@ -59,7 +59,7 @@ public async Task ContinueMergeAsync(bool hierarchyCreated) private async Task ContinueMergeAsyncWorker((string Name, HierarchyNode Parent) dir, bool hierarchyCreated) { var wasExpanded = hierarchyCreated ? dir.Parent.GetIsExpanded() : false; - var missingChildren = new HashSet(dir.Parent.AllChildren); + var missingOnDisk = new HashSet(dir.Parent.AllChildren); var thread = this.project.Site.GetUIThread(); @@ -92,15 +92,9 @@ private async Task ContinueMergeAsyncWorker((string Name, HierarchyNode Pa this.project.CreateSymLinkWatcher(curDir); } - var existing = this.project.FindNodeByFullPath(curDir); - if (existing == null) - { - existing = this.project.AddAllFilesFolder(dir.Parent, curDir + Path.DirectorySeparatorChar, hierarchyCreated); - } - else - { - missingChildren.Remove(existing); - } + var existing = this.project.AddAllFilesFolder(dir.Parent, curDir, hierarchyCreated); + missingOnDisk.Remove(existing); + this.remainingDirs.Push((curDir, existing)); } } @@ -136,7 +130,7 @@ private async Task ContinueMergeAsyncWorker((string Name, HierarchyNode Pa } else { - missingChildren.Remove(existing); + missingOnDisk.Remove(existing); } } @@ -156,7 +150,7 @@ private async Task ContinueMergeAsyncWorker((string Name, HierarchyNode Pa } // remove the excluded children which are no longer there - this.RemoveMissingChildren(missingChildren); + this.RemoveMissingChildren(missingOnDisk); if (hierarchyCreated) { diff --git a/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.FileSystemChange.cs b/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.FileSystemChange.cs index 2afc8c19d..fef237165 100644 --- a/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.FileSystemChange.cs +++ b/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.FileSystemChange.cs @@ -184,7 +184,7 @@ private async Task ChildCreatedAsync(HierarchyNode child) this.project.CreateSymLinkWatcher(this.path); } - var folderNode = this.project.AddAllFilesFolder(parent, this.path + Path.DirectorySeparatorChar); + var folderNode = this.project.AddAllFilesFolder(parent, this.path); var folderNodeWasExpanded = folderNode.GetIsExpanded(); // then add the folder nodes diff --git a/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.cs b/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.cs index ad4e33520..3183b308e 100644 --- a/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.cs +++ b/Nodejs/Product/Nodejs/SharedProject/CommonProjectNode.cs @@ -693,10 +693,12 @@ private HierarchyNode AddAllFilesFile(HierarchyNode curParent, string file) /// private HierarchyNode AddAllFilesFolder(HierarchyNode curParent, string curDir, bool hierarchyCreated = true) { - var folderNode = FindNodeByFullPath(curDir); + var safePath = CommonUtils.EnsureEndSeparator(curDir); + + var folderNode = FindNodeByFullPath(safePath); if (folderNode == null) { - folderNode = CreateFolderNode(new AllFilesProjectElement(curDir, "Folder", this)); + folderNode = CreateFolderNode(new AllFilesProjectElement(safePath, "Folder", this)); AddAllFilesNode(curParent, folderNode); if (hierarchyCreated) From 10ceb660aefb7ef12c6106b82e85f85e44b65ab2 Mon Sep 17 00:00:00 2001 From: Paul van Brenk Date: Wed, 18 Apr 2018 09:43:57 -0700 Subject: [PATCH 3/5] PR feedback --- .../Nodejs/SharedProject/HierarchyIdMap.cs | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs b/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs index 30f4f6c20..77937b7f4 100644 --- a/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs +++ b/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs @@ -16,7 +16,7 @@ internal sealed class HierarchyIdMap /// public uint Add(HierarchyNode node) { - VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); + Debug.Assert(VisualStudio.Shell.ThreadHelper.CheckAccess()); #if DEBUG foreach (var reference in this.nodes.Values) @@ -35,7 +35,9 @@ public uint Add(HierarchyNode node) { idx = this.nodes.Count; } - this.nodes[idx] = new WeakReference(node); + + var addSuccess = this.nodes.TryAdd(idx, new WeakReference(node)); + Debug.Assert(addSuccess, "Failed to add a new item"); return (uint)idx + 1; } @@ -45,22 +47,18 @@ public uint Add(HierarchyNode node) /// public void Remove(HierarchyNode node) { - VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); + Debug.Assert(VisualStudio.Shell.ThreadHelper.CheckAccess()); Debug.Assert(node != null, "Called with null node"); var idx = (int)node.ID - 1; - var removeCheck = this.nodes.TryGetValue(idx, out var weakRef); + var removeCheck = this.nodes.TryRemove(idx, out var weakRef); Debug.Assert(removeCheck, "How did we get an id, which we haven't seen before"); Debug.Assert(weakRef != null, "Double delete is not expected."); + Debug.Assert(weakRef.TryGetTarget(out var found) && object.ReferenceEquals(node, found), "The node has the wrong id, or was GC-ed before."); - var checkReference = weakRef.TryGetTarget(out var found) && object.ReferenceEquals(node, found); - - Debug.Assert(checkReference, "The node has the wrong id."); - - this.nodes[idx] = null; this.freedIds.Push(idx); } @@ -71,18 +69,19 @@ public HierarchyNode this[uint itemId] { get { - VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); + Debug.Assert(VisualStudio.Shell.ThreadHelper.CheckAccess()); - var i = (int)itemId - 1; - if (0 <= i && i < this.nodes.Count) + var idx = (int)itemId - 1; + if (0 <= idx && idx < this.nodes.Count) { - var reference = this.nodes[i]; - if (reference != null && reference.TryGetTarget(out var node)) + if (this.nodes.TryGetValue(idx, out var reference) && reference != null && reference.TryGetTarget(out var node)) { Debug.Assert(node != null); return node; } } + + // This is a valid return value, this gets called by VS after we deleted the item return null; } } From 040d7e1ba84403d8e0baa8a87c22cc07dd132921 Mon Sep 17 00:00:00 2001 From: Paul van Brenk Date: Wed, 18 Apr 2018 16:33:25 -0700 Subject: [PATCH 4/5] More PR feedback --- .../Nodejs/SharedProject/HierarchyIdMap.cs | 31 ++++++++++++------- .../Nodejs/SharedProject/ProjectNode.cs | 5 +-- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs b/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs index 77937b7f4..bd308f7c4 100644 --- a/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs +++ b/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs @@ -8,8 +8,12 @@ namespace Microsoft.VisualStudioTools.Project { internal sealed class HierarchyIdMap { - private readonly ConcurrentDictionary> nodes = new ConcurrentDictionary>(); - private readonly ConcurrentStack freedIds = new ConcurrentStack(); + private readonly ConcurrentDictionary> nodes = new ConcurrentDictionary>(); + private readonly ConcurrentStack freedIds = new ConcurrentStack(); + + public readonly static HierarchyIdMap Instance = new HierarchyIdMap(); + + private HierarchyIdMap() { } /// /// Must be called from the UI thread @@ -33,15 +37,21 @@ public uint Add(HierarchyNode node) #endif if (!this.freedIds.TryPop(out var idx)) { - idx = this.nodes.Count; + idx = this.NextIndex(); } var addSuccess = this.nodes.TryAdd(idx, new WeakReference(node)); Debug.Assert(addSuccess, "Failed to add a new item"); - return (uint)idx + 1; + return idx; + } + + private uint NextIndex() + { + return (uint)this.nodes.Count + 1; } + /// /// Must be called from the UI thread /// @@ -51,7 +61,7 @@ public void Remove(HierarchyNode node) Debug.Assert(node != null, "Called with null node"); - var idx = (int)node.ID - 1; + var idx = node.ID; var removeCheck = this.nodes.TryRemove(idx, out var weakRef); @@ -71,14 +81,11 @@ public HierarchyNode this[uint itemId] { Debug.Assert(VisualStudio.Shell.ThreadHelper.CheckAccess()); - var idx = (int)itemId - 1; - if (0 <= idx && idx < this.nodes.Count) + var idx = itemId; + if (this.nodes.TryGetValue(idx, out var reference) && reference != null && reference.TryGetTarget(out var node)) { - if (this.nodes.TryGetValue(idx, out var reference) && reference != null && reference.TryGetTarget(out var node)) - { - Debug.Assert(node != null); - return node; - } + Debug.Assert(node != null); + return node; } // This is a valid return value, this gets called by VS after we deleted the item diff --git a/Nodejs/Product/Nodejs/SharedProject/ProjectNode.cs b/Nodejs/Product/Nodejs/SharedProject/ProjectNode.cs index 475f00b2d..785c80470 100644 --- a/Nodejs/Product/Nodejs/SharedProject/ProjectNode.cs +++ b/Nodejs/Product/Nodejs/SharedProject/ProjectNode.cs @@ -158,9 +158,6 @@ internal enum EventTriggering /// A project will only try to build if it can obtain a lock on this object private volatile static object BuildLock = new object(); - /// Maps integer ids to project item instances - private HierarchyIdMap itemIdMap = new HierarchyIdMap(); - /// A service provider call back object provided by the IDE hosting the project manager private IServiceProvider site; @@ -590,7 +587,7 @@ public virtual string OutputBaseRelativePath /// /// Gets a collection of integer ids that maps to project item instances /// - internal HierarchyIdMap ItemIdMap => this.itemIdMap; + internal HierarchyIdMap ItemIdMap => HierarchyIdMap.Instance; /// /// Get the helper object that track document changes. From 88c4bc452b68e37b82b656da0b1e4b2b4501ae1a Mon Sep 17 00:00:00 2001 From: Paul van Brenk Date: Wed, 18 Apr 2018 16:42:42 -0700 Subject: [PATCH 5/5] some final cleanup --- .../Product/Nodejs/SharedProject/HierarchyIdMap.cs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs b/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs index bd308f7c4..b6da9cc3d 100644 --- a/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs +++ b/Nodejs/Product/Nodejs/SharedProject/HierarchyIdMap.cs @@ -20,8 +20,8 @@ private HierarchyIdMap() { } /// public uint Add(HierarchyNode node) { - Debug.Assert(VisualStudio.Shell.ThreadHelper.CheckAccess()); - + VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); + Debug.Assert(node != null, "The node added here should never be null."); #if DEBUG foreach (var reference in this.nodes.Values) { @@ -33,7 +33,6 @@ public uint Add(HierarchyNode node) } } } - #endif if (!this.freedIds.TryPop(out var idx)) { @@ -48,16 +47,16 @@ public uint Add(HierarchyNode node) private uint NextIndex() { + // +1 since 0 is not a valid HierarchyId return (uint)this.nodes.Count + 1; } - /// /// Must be called from the UI thread /// public void Remove(HierarchyNode node) { - Debug.Assert(VisualStudio.Shell.ThreadHelper.CheckAccess()); + VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); Debug.Assert(node != null, "Called with null node"); @@ -66,7 +65,7 @@ public void Remove(HierarchyNode node) var removeCheck = this.nodes.TryRemove(idx, out var weakRef); Debug.Assert(removeCheck, "How did we get an id, which we haven't seen before"); - Debug.Assert(weakRef != null, "Double delete is not expected."); + Debug.Assert(weakRef != null, "How did we insert a null value."); Debug.Assert(weakRef.TryGetTarget(out var found) && object.ReferenceEquals(node, found), "The node has the wrong id, or was GC-ed before."); this.freedIds.Push(idx); @@ -79,7 +78,7 @@ public HierarchyNode this[uint itemId] { get { - Debug.Assert(VisualStudio.Shell.ThreadHelper.CheckAccess()); + VisualStudio.Shell.ThreadHelper.ThrowIfNotOnUIThread(); var idx = itemId; if (this.nodes.TryGetValue(idx, out var reference) && reference != null && reference.TryGetTarget(out var node))