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

Remove ContainsKey where relevant #6282

Merged
merged 2 commits into from Mar 31, 2021
Merged

Remove ContainsKey where relevant #6282

merged 2 commits into from Mar 31, 2021

Conversation

@Forgind
Copy link
Member

@Forgind Forgind commented Mar 19, 2021

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.

Copy link
Member Author

@Forgind Forgind left a comment

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.

@@ -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 comment has been minimized.

@Forgind

Forgind Mar 19, 2021
Author Member

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

This comment has been minimized.

@ladipro

ladipro Mar 24, 2021
Contributor

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.

// 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))

This comment has been minimized.

@Forgind

Forgind Mar 19, 2021
Author Member

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

This comment has been minimized.

@Forgind

Forgind Mar 23, 2021
Author Member

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

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

This comment has been minimized.

@Forgind

Forgind Mar 19, 2021
Author Member

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.

@Forgind Forgind marked this pull request as draft Mar 19, 2021
p2

p3

p4

p5

P6

p7
@Forgind Forgind force-pushed the Forgind:remove-contains branch from f36801a to 928968c Mar 23, 2021
@Forgind Forgind marked this pull request as ready for review Mar 23, 2021
@@ -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))

This comment has been minimized.

@ladipro

ladipro Mar 24, 2021
Contributor

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

This comment has been minimized.

@Forgind

Forgind Mar 24, 2021
Author Member

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.

This comment has been minimized.

@Forgind

Forgind Mar 24, 2021
Author Member

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.

// Defaults to false
_projectSupportsReturnsAttribute.TryGetValue(currentProjectOrImport, out NGen<bool> projectSupportsReturnsAttribute);

_projectSupportsReturnsAttribute[currentProjectOrImport] = projectSupportsReturnsAttribute | (target.Returns != null);

This comment has been minimized.

@ladipro

ladipro Mar 24, 2021
Contributor

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

This comment has been minimized.

@Forgind

Forgind Mar 24, 2021
Author Member

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

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

This comment has been minimized.

@ladipro

ladipro Mar 24, 2021
Contributor

nit: Typo retrive.

if (fusionNameToResolvedPath.ContainsKey(strongName))
if (fusionNameToResolvedPath.TryGetValue(strongName, out string fusionName))
{
fusionNameToResolvedPath.TryGetValue(strongName, out string fusionName);
Comment on lines -283 to -285

This comment has been minimized.

@ladipro

ladipro Mar 24, 2021
Contributor

image

@@ -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 comment has been minimized.

@ladipro

ladipro Mar 24, 2021
Contributor

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 Forgind merged commit c2e41ab into dotnet:main Mar 31, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210324.1 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
@Forgind Forgind deleted the Forgind:remove-contains branch Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants