perf(nm): Speedup hardlinks-global link step by 1.5-1.7x (#4532)

* Speedup hardlinks-global link step by 1.5-1.7x

* Adjust mtime comparison to detect immediate file writes

* Adds a test to check store modification in the past

* Adds changelog entry

* chore: update changelog

* chore: remove old changelog entry

Co-authored-by: Maël Nison <[email protected]>
Co-authored-by: merceyz <[email protected]>

Author: 3 people

.yarn/versions/3006ba7c.yml

@@ -0,0 +1,23 @@
+releases:
+  "@yarnpkg/cli": patch
+  "@yarnpkg/plugin-nm": patch
+
+declined:
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-patch"
+  - "@yarnpkg/plugin-pnp"
+  - "@yarnpkg/plugin-pnpm"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/builder"
+  - "@yarnpkg/core"
+  - "@yarnpkg/doctor"

CHANGELOG.md

@@ -14,6 +14,7 @@ Yarn now accepts sponsorships! Please give a look at our [OpenCollective](https:
 
 - The node-modules linker avoids creation of circular symlinks
 - The node-modules linker no longer creates duplicate copies inside of aliased packages
+- Improved performance for `hardlinks-global` `node-modules` linker mode by 1.5x
 
 ## 3.2.4
 

packages/acceptance-tests/pkg-tests-specs/sources/node-modules.test.ts

@@ -1358,7 +1358,7 @@ describe(`Node_Modules`, () => {
     ),
   );
 
-  test(`should recover from changes to the store on next install in nmMode: cas`,
+  test(`should recover from changes to the store on next install in nmMode: hardlinks-global`,
     makeTemporaryEnv(
       {
         dependencies: {
@@ -1394,6 +1394,44 @@ describe(`Node_Modules`, () => {
     ),
   );
 
+  test(`should recover from changes to the store on next install in nmMode: hardlinks-global, when system clock is changed by the user`,
+    makeTemporaryEnv(
+      {
+        dependencies: {
+          dep: `file:./dep`,
+        },
+      },
+      {
+        nodeLinker: `node-modules`,
+        nmMode: `hardlinks-global`,
+      },
+      async ({path, run}) => {
+        await writeJson(ppath.resolve(path, `dep/package.json` as Filename), {
+          name: `dep`,
+          version: `1.0.0`,
+        });
+
+        const originalContent = `The same content`;
+        await xfs.writeFilePromise(ppath.resolve(path, `dep/index.js` as Filename), originalContent);
+
+        await run(`install`);
+
+        const modifiedContent = `The modified content`;
+        const depNmPath = ppath.resolve(path, `node_modules/dep/index.js` as Filename);
+        await xfs.writeFilePromise(depNmPath, modifiedContent);
+        const timeInThePast = new Date(new Date().getTime() - 10000);
+        await xfs.utimesPromise(depNmPath, timeInThePast, timeInThePast);
+
+        await xfs.removePromise(ppath.resolve(path, `node_modules` as Filename));
+
+        await run(`install`);
+
+        const depContent = await xfs.readFilePromise(depNmPath, `utf8`);
+        expect(depContent).toEqual(originalContent);
+      },
+    ),
+  );
+
   test(`should give priority to direct workspace dependencies over indirect regular dependencies`,
     // Despite 'one-fixed-dep' and 'has-bin-entries' depend on 'no-deps:1.0.0',
     // the 'no-deps:2.0.0' should be hoisted to the top-level

packages/plugin-nm/sources/NodeModulesLinker.ts

@@ -20,6 +20,7 @@ const STATE_FILE_VERSION = 1;
 const NODE_MODULES = `node_modules` as Filename;
 const DOT_BIN = `.bin` as Filename;
 const INSTALL_STATE_FILE = `.yarn-state.yml` as Filename;
+const MTIME_ACCURANCY = 1000;
 
 type InstallState = {locatorMap: NodeModulesLocatorMap, locationTree: LocationTree, binSymlinks: BinSymlinkMap, nmMode: NodeModulesMode, mtimeMs: number};
 type BinSymlinkMap = Map<PortablePath, Map<Filename, PortablePath>>;
@@ -695,56 +696,67 @@ async function atomicFileWrite(tmpDir: PortablePath, dstPath: PortablePath, cont
   }
 }
 
-async function copyFilePromise({srcPath, dstPath, srcMode, globalHardlinksStore, baseFs, nmMode, digest}: {srcPath: PortablePath, dstPath: PortablePath, srcMode: number, globalHardlinksStore: PortablePath | null, baseFs: FakeFS<PortablePath>, nmMode: {value: NodeModulesMode}, digest?: string}) {
-  if (nmMode.value === NodeModulesMode.HARDLINKS_GLOBAL && globalHardlinksStore && digest) {
-    const contentFilePath = ppath.join(globalHardlinksStore, digest.substring(0, 2) as Filename, `${digest.substring(2)}.dat` as Filename);
+async function copyFilePromise({srcPath, dstPath, entry, globalHardlinksStore, baseFs, nmMode}: {srcPath: PortablePath, dstPath: PortablePath, entry: DirEntry, globalHardlinksStore: PortablePath | null, baseFs: FakeFS<PortablePath>, nmMode: {value: NodeModulesMode}}) {
+  if (entry.kind === DirEntryKind.FILE) {
+    if (nmMode.value === NodeModulesMode.HARDLINKS_GLOBAL && globalHardlinksStore && entry.digest) {
+      const contentFilePath = ppath.join(globalHardlinksStore, entry.digest.substring(0, 2) as Filename, `${entry.digest.substring(2)}.dat` as Filename);
 
-    let doesContentFileExist;
-    try {
-      const contentDigest = await hashUtils.checksumFile(contentFilePath, {baseFs: xfs, algorithm: `sha1`});
-      if (contentDigest !== digest) {
-        // If file content was modified by the user, or corrupted, we first move it out of the way
-        const tmpPath = ppath.join(globalHardlinksStore, toFilename(`${crypto.randomBytes(16).toString(`hex`)}.tmp`));
-        await xfs.renamePromise(contentFilePath, tmpPath);
+      let doesContentFileExist;
+      try {
+        const stats = await xfs.statPromise(contentFilePath);
+
+        if (stats && (!entry.mtimeMs || stats.mtimeMs > entry.mtimeMs || stats.mtimeMs < entry.mtimeMs - MTIME_ACCURANCY)) {
+          const contentDigest = await hashUtils.checksumFile(contentFilePath, {baseFs: xfs, algorithm: `sha1`});
+          if (contentDigest !== entry.digest) {
+            // If file content was modified by the user, or corrupted, we first move it out of the way
+            const tmpPath = ppath.join(globalHardlinksStore, toFilename(`${crypto.randomBytes(16).toString(`hex`)}.tmp`));
+            await xfs.renamePromise(contentFilePath, tmpPath);
+
+            // Then we overwrite the temporary file, thus restorting content of original file in all the linked projects
+            const content = await baseFs.readFilePromise(srcPath);
+            await xfs.writeFilePromise(tmpPath, content);
+
+            try {
+            // Then we try to move content file back on its place, if its still free
+            // If we fail here, it means that some other process or thread has created content file
+            // And this is okay, we will end up with two content files, but both with original content, unlucky files will have `.tmp` extension
+              await xfs.linkPromise(tmpPath, contentFilePath);
+              entry.mtimeMs = new Date().getTime();
+              await xfs.unlinkPromise(tmpPath);
+            } catch (e) {
+            }
+          } else if (!entry.mtimeMs) {
+            entry.mtimeMs = Math.ceil(stats.mtimeMs);
+          }
+        }
 
-        // Then we overwrite the temporary file, thus restorting content of original file in all the linked projects
-        const content = await baseFs.readFilePromise(srcPath);
-        await xfs.writeFilePromise(tmpPath, content);
+        await xfs.linkPromise(contentFilePath, dstPath);
+        doesContentFileExist = true;
+      } catch (e) {
+        doesContentFileExist = false;
+      }
 
+      if (!doesContentFileExist) {
+        const content = await baseFs.readFilePromise(srcPath);
+        await atomicFileWrite(globalHardlinksStore, contentFilePath, content);
+        entry.mtimeMs = new Date().getTime();
         try {
-          // Then we try to move content file back on its place, if its still free
-          // If we fail here, it means that some other process or thread has created content file
-          // And this is okay, we will end up with two content files, but both with original content, unlucky files will have `.tmp` extension
-          await xfs.linkPromise(tmpPath, contentFilePath);
-          await xfs.unlinkPromise(tmpPath);
+          await xfs.linkPromise(contentFilePath, dstPath);
         } catch (e) {
+          if (e && e.code && e.code == `EXDEV`) {
+            nmMode.value = NodeModulesMode.HARDLINKS_LOCAL;
+            await baseFs.copyFilePromise(srcPath, dstPath);
+          }
         }
       }
-      await xfs.linkPromise(contentFilePath, dstPath);
-      doesContentFileExist = true;
-    } catch (e) {
-      doesContentFileExist = false;
+    } else {
+      await baseFs.copyFilePromise(srcPath, dstPath);
     }
-
-    if (!doesContentFileExist) {
-      const content = await baseFs.readFilePromise(srcPath);
-      await atomicFileWrite(globalHardlinksStore, contentFilePath, content);
-      try {
-        await xfs.linkPromise(contentFilePath, dstPath);
-      } catch (e) {
-        if (e && e.code && e.code == `EXDEV`) {
-          nmMode.value = NodeModulesMode.HARDLINKS_LOCAL;
-          await baseFs.copyFilePromise(srcPath, dstPath);
-        }
-      }
+    const mode = entry.mode & 0o777;
+    // An optimization - files will have rw-r-r permissions (0o644) by default, we can skip chmod for them
+    if (mode !== 0o644) {
+      await xfs.chmodPromise(dstPath, mode);
     }
-  } else {
-    await baseFs.copyFilePromise(srcPath, dstPath);
-  }
-  const mode = srcMode & 0o777;
-  // An optimization - files will have rw-r-r permissions (0o644) by default, we can skip chmod for them
-  if (mode !== 0o644) {
-    await xfs.chmodPromise(dstPath, mode);
   }
 }
 
@@ -756,6 +768,7 @@ type DirEntry = {
   kind: DirEntryKind.FILE;
   mode: number;
   digest?: string;
+  mtimeMs?: number;
 } | {
   kind: DirEntryKind. DIRECTORY;
 } | {
@@ -808,23 +821,33 @@ const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs,
       allEntries = new Map(Object.entries(JSON.parse(await xfs.readFilePromise(entriesJsonPath, `utf8`)))) as Map<PortablePath, DirEntry>;
     } catch (e) {
       allEntries = await getEntriesRecursive();
-      await atomicFileWrite(globalHardlinksStore, entriesJsonPath, Buffer.from(JSON.stringify(Object.fromEntries(allEntries))));
     }
   } else {
     allEntries = await getEntriesRecursive();
   }
 
+  let mtimesChanged = false;
   for (const [relativePath, entry] of allEntries) {
     const srcPath = ppath.join(srcDir, relativePath);
     const dstPath = ppath.join(dstDir, relativePath);
     if (entry.kind === DirEntryKind.DIRECTORY) {
       await xfs.mkdirPromise(dstPath, {recursive: true});
     } else if (entry.kind === DirEntryKind.FILE) {
-      await copyFilePromise({srcPath, dstPath, srcMode: entry.mode, digest: entry.digest, nmMode, baseFs, globalHardlinksStore});
+      const originalMtime = entry.mtimeMs;
+      await copyFilePromise({srcPath, dstPath, entry, nmMode, baseFs, globalHardlinksStore});
+      if (entry.mtimeMs !== originalMtime) {
+        mtimesChanged = true;
+      }
     } else if (entry.kind === DirEntryKind.SYMLINK) {
       await symlinkPromise(ppath.resolve(ppath.dirname(dstPath), entry.symlinkTo), dstPath);
     }
   }
+
+  if (nmMode.value === NodeModulesMode.HARDLINKS_GLOBAL && globalHardlinksStore && mtimesChanged && packageChecksum) {
+    const entriesJsonPath = ppath.join(globalHardlinksStore, packageChecksum.substring(0, 2) as Filename, `${packageChecksum.substring(2)}.json` as Filename);
+    await xfs.removePromise(entriesJsonPath);
+    await atomicFileWrite(globalHardlinksStore, entriesJsonPath, Buffer.from(JSON.stringify(Object.fromEntries(allEntries))));
+  }
 };
 
 /**