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

Ensure trailing slash for string used as key in AssemblyTableInfo's cache #8266

Merged
merged 2 commits into from Jan 10, 2023

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Dec 27, 2022

We do exact string comparisons with this path, and it is user-provided, so if it does not include a slash, we may do more work.

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.
@Forgind Forgind changed the title Ensure trailing slash Ensure trailing slash for string used as key in AssemblyTableInfo's cache Dec 27, 2022
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you discover this? Can you describe the situation where a user might hit the miss?

@@ -3006,8 +3006,7 @@ private AssemblyTableInfo[] GetInstalledAssemblyTableInfo(bool ignoreInstalledAs
{
// Exactly one TargetFrameworkDirectory, so assume it's related to this
// InstalledAssemblyTable.

frameworkDirectory = TargetFrameworkDirectories[0];
frameworkDirectory = FileUtilities.EnsureTrailingSlash(TargetFrameworkDirectories[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be appropriate? It "feels" like it expresses intention more clearly.

Suggested change
frameworkDirectory = FileUtilities.EnsureTrailingSlash(TargetFrameworkDirectories[0]);
frameworkDirectory = FileUtilities.NormalizeForPathComparison(TargetFrameworkDirectories[0]);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that change is reasonable, but if we take it, we'd have to change all the other points that EnsureTrailingSlash rather than NormalizeForPathComparison, or else we'd be ensuring cache misses rather than increasing the chance of cache hits. (I can do that if you'd like, but it's a larger change, and it'd be easier to accidentally miss a case.)

@Forgind
Copy link
Member Author

Forgind commented Jan 3, 2023

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.

@Forgind 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 Jan 9, 2023
@JaynieBai JaynieBai merged commit a53a018 into dotnet:main Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants