Change relative path calculation by Forgind · Pull Request #6311 · dotnet/msbuild

Forgind · 2021-03-30T16:54:24Z

For some reason, the pre-caching change seems to fail in the installer repo at the build step because of a failing relative path calculation. This version seems to work.

benvillalobos · 2021-03-30T21:58:37Z

I assume it's a PR from msbuild into the installer that's failing tests? Can you link it here?

Forgind · 2021-03-30T21:59:46Z

dotnet/installer#10037

Based on some local testing, I currently believe it needs both this and #6301 to go in for it to work. (I also have a couple local changes.)

benvillalobos

First pass review. Any idea what the perf impact is here?

benvillalobos · 2021-03-30T22:29:26Z

Suggested change

return sb.ToString();

return StringBuilderCache.GetStringAndRelease(sb);

I was trying to think of that but didn't come up with it. Thanks!

benvillalobos · 2021-03-30T22:32:37Z

This seems suspicious to me, but I have no evidence off the top of my head 🤔

Seeing if fullPath starts with the current working directory could be another way of determining this, though the current method is faster.

One possible suspicious reason is the (bug) I just found for mac/linux: the path starts with /, which means this will always return false. Now fixed.

ladipro

Can you please add details on path patterns where the old code does not work properly? Thank you!

ladipro · 2021-03-31T07:26:33Z

Calling Split with StringSplitOptions.RemoveEmptyEntries should be a faster way of filtering out empty components.

Had to make it char[] because it seems that Split(char, StringSplitOptions) isn't available on .NET Framework. I don't think ifdefs are worth it here. Still a lot nicer than what I had before; thanks!

ladipro · 2021-03-31T07:35:23Z

What guarantees that the string is not just slashes, meaning that the code won't crash with out of bounds access? The split above uses only the primary directory separator char on the current platform and will accept e.g. forward slashes on Windows.

Forgind · 2021-04-01T21:37:44Z

The old code threw exceptions if the URI was deemed invalid for some reason. Two such cases that stood out to me:
If it contains too many slashes.
If the specified scheme name was invalid. "The scheme name must begin with a letter and must contain only letters, digits, and the characters ".", "+", or "-"." That was too restrictive for, among other things, macs.

ladipro

This is

ladipro · 2021-04-01T21:43:20Z

+            string fullPath = Path.GetFullPath(path);

-            Uri baseUri = new Uri(EnsureTrailingSlash(basePath), UriKind.Absolute); // May throw UriFormatException
+            string[] splitBase = fullBase.Split(new char[] { Path.DirectorySeparatorChar }, StringSplitOptions.RemoveEmptyEntries);


The char array is worth caching. And looks like we already do it in Constants.DirectorySeparatorChar!

ladipro · 2021-04-01T21:49:20Z

+                sb.Append(splitPath[pathI]).Append(Path.DirectorySeparatorChar);
+                pathI++;
+            }
+            sb.Remove(sb.Length - 1, 1);


nit: I would expect

Suggested change

sb.Remove(sb.Length - 1, 1);

sb.Length = sb.Length - 1;

to be slightly faster and also easier to read.

ladipro · 2021-04-01T22:00:07Z

+            while (true)
+            {
+                if (baseI == splitBase.Length)
+                {
+                    if (pathI == splitPath.Length)
+                    {
+                        return ".";
+                    }
+                    break;
+                }
+                else if (pathI == splitPath.Length)
+                {
+                    break;
+                }
+                else if (splitBase[baseI].Equals(splitPath[pathI], PathComparison))
+                {
+                    baseI++;
+                    pathI++;
+                }
+                else
+                {
+                    break;
+                }
+            }


I would find the following much easier to read. As long as it's equivalent, please double-check:

Suggested change

while (true)

{

if (baseI == splitBase.Length)

{

if (pathI == splitPath.Length)

{

return ".";

}

break;

}

else if (pathI == splitPath.Length)

{

break;

}

else if (splitBase[baseI].Equals(splitPath[pathI], PathComparison))

{

baseI++;

pathI++;

}

else

{

break;

}

}

while (baseI < splitBase.Length && pathI < splitPath.Length && splitBase[baseI].Equals(splitPath[pathI], PathComparison))

{

baseI++;

pathI++;

}

if (baseI == splitBase.Length && pathI == splitPath.Length)

{

return ".";

}

}

ladipro · 2021-04-02T11:25:39Z

+            int baseI = 0;
+            int pathI = 0;


nit (last one, promise!): Since the two integers are always incremented together, consider having only one and changing the while (baseI < ... and while (pathI < ... below to for loops. Totally optional, no perf impact, just a matter of subjective taste.

Yep! This PR came out unusually janky, so I appreciate all the refactoring suggestions.

benvillalobos

I like where this PR ended up. I did some rough testing to see which was faster and it looks like this version is 👍

benvillalobos · 2021-04-02T18:19:24Z

+            {
+                return ".";
+            }
+            // If the paths have no component in common, the only valid relative path is the full path.


hyper-nit-9000: newline here.

Forgind · 2021-04-02T18:48:58Z

Thanks @benvillalobos for the perf check!

benvillalobos reviewed Mar 30, 2021

View reviewed changes

Change relative path calculation

437e0f5

ladipro reviewed Mar 31, 2021

View reviewed changes

PR feedback

1d4fea6

Forgind force-pushed the change-relative-path-calculation branch from d770c12 to 1d4fea6 Compare March 31, 2021 14:09

Forgind mentioned this pull request Apr 1, 2021

Introduce pre-caching into installer dotnet/installer#10037

Merged

ladipro reviewed Apr 1, 2021

View reviewed changes

feedback

d8c9ccc

ladipro approved these changes Apr 2, 2021

View reviewed changes

Forgind added 2 commits April 2, 2021 06:18

refactor

21095b7

Fix index

457557b

benvillalobos approved these changes Apr 2, 2021

View reviewed changes

Forgind commented Apr 2, 2021

View reviewed changes

Comment thread src/Shared/FileUtilities.cs

Update src/Shared/FileUtilities.cs

d8dbbee

Forgind merged commit 1cfebe4 into dotnet:main Apr 2, 2021

Forgind deleted the change-relative-path-calculation branch April 2, 2021 22:16

Forgind mentioned this pull request Jun 4, 2021

Fix MakeRelative regression in v16.10 (rebased to vs16.10 branch) #6513

Merged

	return sb.ToString();
	return StringBuilderCache.GetStringAndRelease(sb);

Conversation

Forgind commented Mar 30, 2021

Uh oh!

benvillalobos commented Mar 30, 2021

Uh oh!

Forgind commented Mar 30, 2021

Uh oh!

benvillalobos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ladipro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind commented Apr 1, 2021

Uh oh!

ladipro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benvillalobos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Forgind commented Apr 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants