From a53a0182a5f5967d4db8015bf1d3c7745d8ffc14 Mon Sep 17 00:00:00 2001 From: Forgind <12969783+Forgind@users.noreply.github.com> Date: Mon, 9 Jan 2023 19:09:19 -0800 Subject: [PATCH] Ensure trailing slash for string used as key in AssemblyTableInfo's cache (#8266) * Ensure trailing slash We do exact string comparisons with this path, and it is user-provided, so if it does not include a slash, we may do slightly more work. *How did you discover this?* I was working on a larger change around caching in RAR and trying to get a general idea for how things worked when I noticed this. I finished the larger change, but it didn't actually seem to really affect performance, so I don't think it's worth a PR. I started another change as well that might improve perf, but I haven't finished that one yet. *Can you describe the situation where a user might hit the miss?* This is a bit narrow, but if you pass in exactly one TargetFrameworkDirectory, it doesn't end in a slash, and ou also pass in one or more AssemblyTables with FrameworkDirectory metadata, that un-normalized directory will be part of the key into s_cachedRedistList here. If you then specify one or more LatestTargetFrameworkDirectories, those will be compared against that directory here. Looking at that more, though, I do think my current change is insufficient, and I should apply normalization across the board. I had tried to just align with the other code path in that method that was doing some light normalization, but it looks like we aren't even consistent on whether we try to normalize the path. I'm sure it isn't a huge perf loss, but it still is slightly wasteful. I'm a little split between normalizing every instance in which we make these paths versus normalizing just before using the key; the latter is easier but would probably be a bit more allocate-y. Maybe not, though? I'll look more. --- src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs | 9 +-------- src/Tasks/RedistList.cs | 4 ++-- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs b/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs index d943ca958c1..9dc674cb6ac 100644 --- a/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs +++ b/src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs @@ -2998,23 +2998,16 @@ private AssemblyTableInfo[] GetInstalledAssemblyTableInfo(bool ignoreInstalledAs // Whidbey behavior was to accept a single TargetFrameworkDirectory, and multiple // InstalledAssemblyTables, under the assumption that all of the InstalledAssemblyTables // were related to the single TargetFrameworkDirectory. If inputs look like the Whidbey - // case, let's make sure we behave the same way. - + // case, let's make sure we behave the same way. Otherwise, use non-empty metadata. if (String.IsNullOrEmpty(frameworkDirectory)) { if (TargetFrameworkDirectories?.Length == 1) { // Exactly one TargetFrameworkDirectory, so assume it's related to this // InstalledAssemblyTable. - frameworkDirectory = TargetFrameworkDirectories[0]; } } - else - { - // The metadata on the item was non-empty, so use it. - frameworkDirectory = FileUtilities.EnsureTrailingSlash(frameworkDirectory); - } tableMap[installedAssemblyTable.ItemSpec] = new AssemblyTableInfo(installedAssemblyTable.ItemSpec, frameworkDirectory); } diff --git a/src/Tasks/RedistList.cs b/src/Tasks/RedistList.cs index 6d2862fac49..2fdd200203a 100644 --- a/src/Tasks/RedistList.cs +++ b/src/Tasks/RedistList.cs @@ -918,8 +918,8 @@ internal class AssemblyTableInfo : IComparable internal AssemblyTableInfo(string path, string frameworkDirectory) { - Path = path; - FrameworkDirectory = frameworkDirectory; + Path = FileUtilities.NormalizeForPathComparison(path); + FrameworkDirectory = FileUtilities.NormalizeForPathComparison(frameworkDirectory); } internal string Path { get; }