Performance: Parallelize recursive directory copying

[ivan-ottinger]

Related issues

• Related to STU-980

Proposed Changes

• parallelize recursive directory copying

Compared to the current trunk I have measured about 16% speed-up of the site creation process on macOS (about 1.2 s saving). On Windows the percentage is smaller (about 6%) - as the overall site creation process takes longer there.

Testing Instructions

1. Check out the PR branch and build the app with npm install && npm start.
2. Create several new sites.
3. The process should work correctly, the sites should get created and run without any issues.

Pre-merge Checklist

• Have you checked for TypeScript, React or other console errors?

[fredrikekelund]

Clever solution, @ivan-ottinger 👍 We should add p-limit@^3.1.0 to our package.json file before landing this PR, but I'll still approve it now to unblock you.

I went on a tangent exploring how we might use the limit.map API which is available in more recent version of p-limit, but ended up finding that it actually breaks in several ways (see my review comment).

[fredrikekelund]

I was going to suggest we do something like my suggestion below, but apparently, there are several issues with this solution (see bottom):

const recursiveCopyLimiter = pLimit( 100 );

export async function recursiveCopyDirectory(
	source: string,
	destination: string
): Promise< void > {
	await fsPromises.mkdir( destination, { recursive: true } );
	const entries = await fsPromises.readdir( source, { withFileTypes: true } );

	await recursiveCopyLimiter.map( entries, async ( entry ) => {
		const sourcePath = path.join( source, entry.name );
		const destinationPath = path.join( destination, entry.name );

		if ( entry.isDirectory() ) {
			await recursiveCopyDirectory( sourcePath, destinationPath );
		} else if ( entry.isFile() ) {
			await fsPromises.copyFile( sourcePath, destinationPath );
		}
	} );
}

• We should add p-limit to package.json, but something with how the more recent versions use ESM doesn't play nicely with our Vite config, and I end up getting errors like "pLimit is not a function" when testing my suggestion. Frustrating…
• I thought that moving the limiter definition outside the function to control the overall parallelism was smart, but that won't work (see Recursive use of p-limit may cause silent task loss when inner tasks depend on limited concurrency sindresorhus/p-limit#91)

The solution to these problems, as I see it, is to leave the code as is and add p-limit@^3.1.0 to our package.json file.

[fredrikekelund]

Out of curiosity, did you compare performance between this approach and using the copy function from the fs-extra module, @ivan-ottinger?

We're using that API elsewhere in our codebase. Might be nice to standardize on the most performant approach.

[wpmobilebot]

📊 Performance Test Results

Comparing d13094b vs trunk

site-editor

Metric	trunk	d13094b	Diff	Change
load	2832.00 ms	2839.00 ms	+7.00 ms	🔴 0.2%

site-startup

Metric	trunk	d13094b	Diff	Change
siteCreation	7091.00 ms	8088.00 ms	+997.00 ms	🔴 14.1%
siteStartup	3941.00 ms	3946.00 ms	+5.00 ms	🔴 0.1%

---

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change

[ivan-ottinger]

Out of curiosity, did you compare performance between this approach and using the copy function from the fs-extra module, @ivan-ottinger?

We're using that API elsewhere in our codebase. Might be nice to standardize on the most performant approach.

Good point! I have compared all three approaches on macOS. Using current p-limit implamentation is slightly faster than the copy function from the fs-extra module. But the difference is quite small. Because of that I think it is worth switching to the copy function. The code will be more maintainable and we don't need to import p-limit.

→ I have made the change in fbb9352.

[fredrikekelund]

LGTM 👍 Thanks for taking another look and improving consistency.

[gcsecsey]

Thanks @ivan-ottinger for improving the performance! The changes LGTM, and I found no regressions when testing both on Mac and Windows. 👍

[youknowriad]

Looking at the graphs in codevitals, this PR made the site creation a little bit slower it seems.

[ivan-ottinger]

Looking at the graphs in codevitals, this PR made the site creation a little bit slower it seems.

That's interesting. Thanks for sharing, Riad.

I checked codevitals and it indeed shows an increase there:

I went ahead and built the app on both commits to test things manually as well. There's an average of +0.5 seconds for fs-extra compared to the previous approach:

 Method                      Avg     Median        Min        Max
  -----------------------------------------------------------------
  old-sequential              7.10       7.15       6.35       8.28
    (68a1d67)
  fs-extra (924c115)          7.60       7.53       7.25       8.90
    +7.0%

(run 5 site creations per method)

Then I let Claude write a benchmark script to measure just the speed of WordPress files copy in isolation using different methods. Here are the results:

Results (ms)
------------------------------------------------------------------------
Method                      Avg     Median        Min        Max
------------------------------------------------------------------------
old-sequential            704.6      714.2      645.5      834.9  (baseline)
fs-extra-copy             444.8      438.6      383.4      610.4  -36.9%
parallel-copy             369.7      347.4      312.3      493.0  -47.5%
========================================================================

(run 10 times per method)

This shows that fs-extra is faster than the old sequential copy when it is performed in isolation. There must be something in that approach making the whole site creation slower.

parallel-copy is approach which I experimented with while working on this PR previously.

export async function recursiveCopyDirectory(
	source: string,
	destination: string
): Promise< void > {
	await fsPromises.mkdir( destination, { recursive: true } );

	const entries = await fsPromises.readdir( source, { withFileTypes: true } );

	await Promise.all(
		entries.map( ( entry ) => {
			const sourcePath = path.join( source, entry.name );
			const destinationPath = path.join( destination, entry.name );

			if ( entry.isDirectory() ) {
				return recursiveCopyDirectory( sourcePath, destinationPath );
			}
			if ( entry.isFile() ) {
				return fsPromises.copyFile( sourcePath, destinationPath );
			}
		} )
	);
}

But then we switched to fs-extra which got merged at the end.

I will create a new PR that will use the parallel-copy which shows the highest potential for performance boost. Here's how it compares when manually testing it with the built app against other two methods:

 Method                      Avg     Median        Min        Max
  -----------------------------------------------------------------
  old-sequential              7.10       7.15       6.35       8.28
    (68a1d67)
  fs-extra (924c115)          7.60       7.53       7.25       8.90
    +7.0%
  parallel-copy (new)         6.24       6.21       6.01       6.90
    -12.1%
  =================================================================

(run 5 site creations per method)

---

Btw, is there an easy way how we could run codevitals on a branch that hasn't been merged yet? Thanks!

[youknowriad]

Btw, is there an easy way how we could run codevitals on a branch that hasn't been merged yet?

If I'm not wrong, I initially had the metrics job run on each PR and you can dive into the logs to check. We do this in Gutenberg and else, if we accept a few more minutes to wait before PR merges

[wojtekn]

@ivan-ottinger, the job which runs performance metrics on branch posts a GH comment with results. The one under this PR shows ~1s decrease for site creation.