Speed up file installation on Windows

internal/smartlink/smartlink.go

@@ -28,8 +28,12 @@ func LinkContents(src, dest string) error {
 	if err != nil {
 		return errs.Wrap(err, "Reading dir %s failed", src)
 	}
+	// src and dest are already resolved above, so recurse via link() which does not
+	// re-resolve each entry. On Windows resolving a path is a syscall (GetLongPathName),
+	// so resolving once per tree instead of once per file is a large speed-up when
+	// installing runtimes that contain many files.
 	for _, entry := range entries {
-		if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil {
+		if err := link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil {
 			return errs.Wrap(err, "Link failed")
 		}
 	}
@@ -40,23 +44,37 @@ func LinkContents(src, dest string) error {
 // Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure),
 // which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks.
 func Link(src, dest string) error {
-	originalSrc := src
+	resolvedDest, err := fileutils.ResolveUniquePath(dest)
+	if err != nil {
+		return errs.Wrap(err, "Could not resolve dest path")
+	}
 
-	var err error
-	src, dest, err = resolvePaths(src, dest)
+	// Resolve the parent of src rather than src itself and rejoin the base name. ResolveUniquePath
+	// dereferences symlinks, but link() needs to see whether the leaf is itself a symlink, so we must
+	// not dereference it here. For a non-symlink leaf this is equivalent to resolving src directly.
+	srcParent, err := fileutils.ResolveUniquePath(filepath.Dir(src))
 	if err != nil {
-		return errs.Wrap(err, "Could not resolve src and dest paths")
+		return errs.Wrap(err, "Could not resolve src path")
 	}
 
+	return link(filepath.Join(srcParent, filepath.Base(src)), resolvedDest)
+}
+
+// link recursively links src into dest. Unlike Link, it assumes src and dest are already resolved to
+// unique paths and does NOT re-resolve them for every entry it visits. Path resolution is a syscall
+// per path on Windows (GetLongPathName), so resolving once per tree rather than once per file makes
+// installing runtimes with many files dramatically faster. Descendant paths are constructed by joining
+// already-resolved parents with real (long) entry names, so they need no further resolution.
+func link(src, dest string) error {
 	if fileutils.IsDir(src) {
-		if fileutils.IsSymlink(originalSrc) {
-			// If the original src is a symlink, the resolved src is no longer a symlink and could point
+		if fileutils.IsSymlink(src) {
+			// If src is a symlink, the resolved src is no longer a symlink and could point
 			// to a parent directory, resulting in a recursive directory structure.
-			// Avoid any potential problems by simply linking the original link to the target.
+			// Avoid any potential problems by simply linking the symlink to the target.
 			// Links to directories are okay on Linux and macOS, but will fail on Windows.
 			// If we ever get here on Windows, the artifact being deployed is bad and there's nothing we
 			// can do about it except receive the report from Rollbar and report it internally.
-			return linkFile(originalSrc, dest)
+			return linkFile(src, dest)
 		}
 
 		if err := fileutils.Mkdir(dest); err != nil {
@@ -67,13 +85,22 @@ func Link(src, dest string) error {
 			return errs.Wrap(err, "could not read directory %s", src)
 		}
 		for _, entry := range entries {
-			if err := Link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil {
+			if err := link(filepath.Join(src, entry.Name()), filepath.Join(dest, entry.Name())); err != nil {
 				return errs.Wrap(err, "sub link failed")
 			}
 		}
 		return nil
 	}
 
+	// A symlink whose target is a file: link to the resolved target to preserve pre-existing behavior.
+	if fileutils.IsSymlink(src) {
+		resolvedSrc, err := fileutils.ResolveUniquePath(src)
+		if err != nil {
+			return errs.Wrap(err, "could not resolve src path %s", src)
+		}
+		src = resolvedSrc
+	}
+
 	destDir := filepath.Dir(dest)
 	if err := fileutils.MkdirUnlessExists(destDir); err != nil {
 		return errs.Wrap(err, "could not create directory %s", destDir)
@@ -87,6 +114,12 @@ func Link(src, dest string) error {
 	}
 
 	if err := linkFile(src, dest); err != nil {
+		// Another artifact may have created the same file concurrently between the check above and now.
+		// Treat that the same as the "already exists" case rather than failing the whole install.
+		if os.IsExist(err) {
+			logging.Warning("Skipping linking '%s' to '%s' as it already exists", src, dest)
+			return nil
+		}
 		return errs.Wrap(err, "could not link %s to %s", src, dest)
 	}
 	return nil

pkg/runtime/depot.go

@@ -214,10 +214,14 @@ func (d *depot) MarkPrivate(id strfmt.UUID) error {
 
 // DeployViaLink will take an artifact from the depot and link it to the target path.
 // It should return deployment info to be used for tracking the artifact.
+//
+// Unlike DeployViaCopy this does NOT hold d.fsMutex for the duration of the operation, so multiple
+// artifacts can be linked into the runtime concurrently (the install worker pool runs several at once).
+// This is safe because linking touches no shared in-memory depot state (d.Exists guards its own access
+// via mapMutex, and d.depotPath is immutable after construction), directory creation is idempotent, and
+// smartlink treats an already-existing target as a skip. This is a major win on Windows where each file
+// link is an individual, relatively expensive syscall.
 func (d *depot) DeployViaLink(id strfmt.UUID, relativeSrc, absoluteDest string) (*deployment, error) {
-	d.fsMutex.Lock()
-	defer d.fsMutex.Unlock()
-
 	if exists, _ := d.Exists(id); !exists {
 		return nil, errs.New("artifact not found in depot")
 	}
@@ -262,6 +266,11 @@ func (d *depot) DeployViaLink(id strfmt.UUID, relativeSrc, absoluteDest string)
 
 // DeployViaCopy will take an artifact from the depot and copy it to the target path.
 // It should return deployment info to be used for tracking the artifact.
+//
+// This intentionally holds d.fsMutex for the duration of the copy. Unlike linking, copying a file that
+// two artifacts both provide would race on os.Create and could corrupt the destination, so copies are
+// serialized. The copy path is only used when an artifact needs file transforms, hard links are
+// unsupported, or portable mode is requested, so it is the less common case.
 func (d *depot) DeployViaCopy(id strfmt.UUID, relativeSrc, absoluteDest string) (*deployment, error) {
 	d.fsMutex.Lock()
 	defer d.fsMutex.Unlock()