Remove ContainsKey where relevant #6282

Forgind · 2021-03-19T19:10:33Z

Each instance of ContainsKey that I deleted was followed by querying the same dictionary for the same key. These can be done at the same time, so I replaced them.

I do not expect that this will have any substantial user impact, but it theoretically improves perf, and it's a pet peeve of mine.

Forgind

I have a few unrelated changes like in RoslynCodeTaskFactory, but I think it's all fairly straightforward. Also, some of my variable names are bad. If you have better ideas, please suggest them.

Forgind · 2021-03-19T19:16:54Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

@@ -484,10 +484,7 @@ internal bool AcquireAndSetUpHost(HandshakeOptions hostContext, INodePacketFacto
        /// </summary>
        internal void DisconnectFromHost(HandshakeOptions hostContext)
        {
-            ErrorUtilities.VerifyThrow(_nodeIdToPacketFactory.ContainsKey((int)hostContext) && _nodeIdToPacketHandler.ContainsKey((int)hostContext), "Why are we trying to disconnect from a context that we already disconnected from?  Did we call DisconnectFromHost twice?");
-
-            _nodeIdToPacketFactory.Remove((int)hostContext);


This change technically means removing hostContext isn't atomic, but I don't think that's an issue.

I would slightly prefer the actual logic to be on its own lines and not part the VerifyThrow() call. It makes it easier to read for me when error handling does not mutate the state.

Forgind · 2021-03-19T19:35:38Z

src/Tasks/Copy.cs

+                // It's very common for a lot of files to be copied to the same folder. 
+                // Eg., "c:\foo\a"->"c:\bar\a", "c:\foo\b"->"c:\bar\b" and so forth.
+                // We don't want to check whether this folder exists for every single file we copy. So store which we've checked.
+                _directoriesKnownToExist.TryAdd(destinationFolder, true))


Note the absence of !. Finding it in the dictionary means it cannot be added, so ContainsKey and TryAdd return opposite values here.

It appears this prevented the creation of some directories where necessary, so I removed it.

Forgind · 2021-03-19T19:36:54Z

src/Tasks/ManifestUtil/ApplicationManifest.cs

@@ -421,25 +421,25 @@ private void ValidateCom()
                            if (!String.IsNullOrEmpty(comInfo.ClsId))
                            {
                                string key = comInfo.ClsId.ToLowerInvariant();
-                                if (!clsidList.ContainsKey(key))


There are numerous cases in which we could eliminate another dictionary access via TryAdd or some similar method, but that isn't available on Framework, sadly.

p2 p3 p4 p5 P6 p7

ladipro · 2021-03-24T09:19:02Z

src/Build/Construction/Solution/SolutionProjectGenerator.cs

@@ -783,7 +783,7 @@ private void EvaluateAndAddProjects(List<ProjectInSolution> projectsInOrder, Lis
                AddTraversalTargetForProject(traversalInstance, project, projectConfiguration, "Publish", null, canBuildDirectly);

                // Add any other targets specified by the user that were not already added
-                foreach (string targetName in _targetNames.Where(i => !traversalInstance.Targets.ContainsKey(i)))
+                foreach (string targetName in _targetNames.Except(traversalInstance.Targets.Keys, StringComparer.OrdinalIgnoreCase))


Was the old code really doing case-insensitive comparison? traversalInstance.Targets looks case sensitive.

I'd initially had it case-sensitive, and it failed in a way that made this look guilty, but I found a couple other bugs since then. Let me try again and get back to you.

SolutionProjectGenerator_Tests.IllegalUserTargetNamesDoNotThrow(forceCaseDifference: True) was the test that failed. Looking at it, it explicitly changes the casing to be different and still expects it to work the same. I would assume that if we should do case-insensitive comparisons at one point, we should do it everywhere in SolutionProjectGenerator when looking at targets' names, though I'm open to being wrong.

ladipro · 2021-03-24T09:29:38Z

src/Build/Evaluation/Evaluator.cs

+                            // Defaults to false
+                            _projectSupportsReturnsAttribute.TryGetValue(currentProjectOrImport, out NGen<bool> projectSupportsReturnsAttribute);
+
+                            _projectSupportsReturnsAttribute[currentProjectOrImport] = projectSupportsReturnsAttribute | (target.Returns != null);


nit: Is the bit-wise OR intentional here? Would it work as well with || which is more natural for bool values?

I was just maintaining how it was before, but I agree that's more natural. Thanks!

ladipro · 2021-03-24T09:42:08Z

src/Build/Logging/ParallelLogger/ParallelLoggerHelpers.cs

@@ -47,31 +47,26 @@ internal void AddProjectStartedEvent(ProjectStartedEventArgs e, bool requireTime
                    int projectTargetKeyLocal = 1;
                    int projectIncrementKeyLocal;
                    // If we haven't seen this project before (by full path) then
-                    // allocate a new key for it and save it away
-                    if (!_projectKey.ContainsKey(e.ProjectFile))
+                    // allocate a new key for it and save it away. Otherwise, retrive it.


nit: Typo retrive.

ladipro · 2021-03-24T10:27:45Z

src/Tasks/AssemblyDependency/GlobalAssemblyCache.cs

-                    if (fusionNameToResolvedPath.ContainsKey(strongName))
+                    if (fusionNameToResolvedPath.TryGetValue(strongName, out string fusionName))
                    {
-                        fusionNameToResolvedPath.TryGetValue(strongName, out string fusionName);


ladipro · 2021-03-24T10:38:58Z

src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcTaskHost.cs

@@ -484,10 +484,7 @@ internal bool AcquireAndSetUpHost(HandshakeOptions hostContext, INodePacketFacto
        /// </summary>
        internal void DisconnectFromHost(HandshakeOptions hostContext)
        {
-            ErrorUtilities.VerifyThrow(_nodeIdToPacketFactory.ContainsKey((int)hostContext) && _nodeIdToPacketHandler.ContainsKey((int)hostContext), "Why are we trying to disconnect from a context that we already disconnected from?  Did we call DisconnectFromHost twice?");
-
-            _nodeIdToPacketFactory.Remove((int)hostContext);


I would slightly prefer the actual logic to be on its own lines and not part the VerifyThrow() call. It makes it easier to read for me when error handling does not mutate the state.

Forgind commented Mar 19, 2021

View reviewed changes

Forgind marked this pull request as draft March 19, 2021 22:58

Reduce dictionary accesses

928968c

p2 p3 p4 p5 P6 p7

Forgind force-pushed the remove-contains branch from f36801a to 928968c Compare March 23, 2021 21:45

Forgind marked this pull request as ready for review March 23, 2021 22:03

cdmihai approved these changes Mar 23, 2021

View reviewed changes

ladipro approved these changes Mar 24, 2021

View reviewed changes

PR comments

52845ea

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 29, 2021

Forgind merged commit c2e41ab into dotnet:main Mar 31, 2021

Forgind deleted the remove-contains branch March 31, 2021 06:48

Forgind mentioned this pull request May 19, 2021

Building a subset of a VS solution at the command line causes MSB4057 #6465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ContainsKey where relevant #6282

Remove ContainsKey where relevant #6282

Forgind commented Mar 19, 2021 •

edited

Forgind left a comment

Forgind Mar 19, 2021

ladipro Mar 24, 2021

Forgind Mar 19, 2021

Forgind Mar 23, 2021

Forgind Mar 19, 2021

ladipro Mar 24, 2021

Forgind Mar 24, 2021

Forgind Mar 24, 2021

ladipro Mar 24, 2021

Forgind Mar 24, 2021

ladipro Mar 24, 2021

ladipro Mar 24, 2021

ladipro Mar 24, 2021

Remove ContainsKey where relevant #6282

Remove ContainsKey where relevant #6282

Conversation

Forgind commented Mar 19, 2021 • edited

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind commented Mar 19, 2021 •

edited