From 92b6e11c0505c494089d7981cffbb354c9d63731 Mon Sep 17 00:00:00 2001 From: icanhasmath Date: Mon, 6 Jul 2026 15:41:19 -0500 Subject: [PATCH 1/2] Resolve link paths once per tree instead of once per file smartlink.Link resolved both src and dest via ResolveUniquePath for every entry it recursed into. On Windows ResolveUniquePath calls GetLongPathName, which is a syscall, so installing a runtime with many files performed two extra syscalls per file - a major contributor to slow Windows installs. The tree root is already resolved by LinkContents (and now by the public Link), and descendant paths are built by joining an already-resolved parent with real (long) entry names, so they need no further resolution. Split the recursion into an unexported link() worker that assumes resolved inputs and does not re-resolve, keeping resolution only for the rare symlink-to-file case where the pre-existing behavior links to the dereferenced target. Also treat an "already exists" error from linkFile the same as the existing TargetExists skip, so concurrent deploys of a shared file degrade to the documented "first one wins" behavior rather than failing. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/smartlink/smartlink.go | 53 ++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/internal/smartlink/smartlink.go b/internal/smartlink/smartlink.go index c101abde5e..764b36de2b 100644 --- a/internal/smartlink/smartlink.go +++ b/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 From 1192e7003d15a2b86b269e1c2db125992382109e Mon Sep 17 00:00:00 2001 From: icanhasmath Date: Mon, 6 Jul 2026 15:42:21 -0500 Subject: [PATCH 2/2] Deploy linked artifacts concurrently instead of serializing them DeployViaLink held the depot-wide fsMutex for the entire link operation, so even though the install worker pool runs several artifacts at once, the actual file linking was serialized to one artifact at a time. On Windows, where each link is an individual (relatively slow) syscall, this made the install phase far slower than it needed to be. Linking touches no shared in-memory depot state (Exists guards its own map via mapMutex and depotPath is immutable after construction), directory creation is idempotent, and smartlink now treats an already-existing target as a skip, so it is safe to run without the lock. Drop it from DeployViaLink so linking parallelizes across the worker pool. DeployViaCopy keeps the lock: two artifacts copying the same file would race on os.Create and could corrupt the destination. That path is only used for artifacts needing file transforms, when hard links are unsupported, or in portable mode, so it remains the less common case. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/runtime/depot.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/runtime/depot.go b/pkg/runtime/depot.go index a558400442..ddfd477a13 100644 --- a/pkg/runtime/depot.go +++ b/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()