Async bootstrap

[Aerijo]

Requirements for Adding, Changing, or Removing a Feature

• Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• The pull request must contribute a change that has been endorsed by the maintainer team. See details in the template below.
• The pull request must update the test suite to exercise the updated functionality. For guidance, please see https://flight-manual.atom.io/hacking-atom/sections/writing-specs/.
• After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see https://github.com/atom/.github/tree/master/CONTRIBUTING.md#pull-requests.

Issue or RFC Endorsed by Atom's Maintainers

None yet. This PR should show if it's worth looking into.

Description of the Change

Attempts to make key parts of the build script async. Many stages of the build do not depend on others, but a lot of synchronous file operations are used. This PR converts many of them to use promises, to allow multiple steps to run concurrently. This is expected to be an improvement because the build seems to be IO heavy.

This change is relatively conservative, as there are still parts that use synchronous calls. I also haven't worked out all the dependencies yet, so it's possible some steps may still be run concurrently.

Alternate Designs

I noticed @aminya is trying out worker threads. It would be interesting to see if that approach offers any significant gains over Promises.

Possible Drawbacks

It makes logging more difficult :(. It's not a problem though, the work in #21288 theoretically supports logging for async jobs (even across worker threads).

Verification Process

CI passes and time for bootstrap + build is decreased.

Edit: The initial approach had a lot of changes, and something broke. I've restricted it to just the bootstrap right now.

Release Notes

NA

[aminya]

I noticed @aminya is trying out worker threads. It would be interesting to see if that approach offers any significant gains over Promises.

Nodejs is single-threaded by default which means "any Nodejs code" will eventually run on the main thread. A promise only helps if the operation happens outside of Nodejs. That includes I/O intensive work (such as reading/writing/downloading/etc), etc.

Multi-threading is completely different since you spread the "Nodejs code" across different threads. For example, you can compile apm in parallel to doing something else.

    await Promise.all([
      transpilePackagesWithCustomTranspilerPaths(),
      transpileBabelPaths(),
      transpileCoffeeScriptPaths(),
      transpileCsonPaths(),
      transpilePegJsPaths(),
    ]);

Here you just changing the code that runs on the main thread "async" and then awaiting it later, and you expect it to become faster. This will not make any difference, and it will be probably worse since Nodejs needs to switch between all of these operations while they all "run on the main thread"

[Aerijo]

Main change (git does not like wrapping sections of code into a function it seems).

Adds the JOBS=max environment config for node-gyp.

Makes the script dependencies install concurrently with apm + apm install. The other parts are too quick to be worth making concurrent (except perhaps the cleanDependencies function; the effect can only be seen locally though, so will need to run more tests with that and maybe make a followup PR).

[Aerijo]

Making the processes async helps a little, but they compete for bandwidth and disk usage so running them in parallel (with async or worker threads) isn't as useful as it first appears. The main performance boost here comes from the JOBS=max setting (added to match the extra changes in #21308).

[Aerijo]

Initial results indicate this out performs #21308 on mac and Linux builds, but is slower on Windows. But this is only after a single try for both of them, and the CI stage times are highly variable (at least +- a few minutes, which is all they are different by). If they are to be compared, both should be run a few more times to get a better sense of the performance.

Having said that, the difference should not be significant at all. In the end, both are limited by the performance of the two child processes.

[aminya]

Initial results indicate this out performs #21308 on mac and Linux builds, but is slower on Windows.

Just adding a note about #21308. It does not have the parallelization of the build yet, and it is only for bootstrap.

To see the effect of threads on the build you should check this:

MacOS builds in 7:15. It is around 6 min faster than your implementation.
Linux builds in 11:45. It is arund 2 min faster than your implementation.

[Aerijo]

Just adding a note about #21308. It does not have the parallelization of the build yet, and it is only for bootstrap.

What a coincidence, the same applies for this PR.

[Aerijo]

This currently has a race condition; the npmBinPath used to install apm depends on the npm installed by installing script dependencies, so making the two processes run in parallel changes behaviour. It is probably better to work on removing the dependency on npm first before continuing with this.

I do not think this race is the cause of the test failures though; they seem to be flaky (if it was a problem with this PR I would have expected an import error or build time issue).

[aminya]

This currently has a race condition;

No that is not correct. The scripts use the externally installed npm for installing apm.

atom/script/config.js

Line 110 in 0628ba5

if (process.env.NPM_BIN_PATH) return process.env.NPM_BIN_PATH;

[Aerijo]

...only if that variable is set though. This runs on more than just CI.

[aminya]

Outside of CI the system npm is used:

atom/script/config.js

Line 122 in 0628ba5

: npmBinName;

[Aerijo]

No, the local one is preferred if it exists

atom/script/config.js

Lines 113 to 122 in 0628ba5

	const localNpmBinPath = path.resolve(
	repositoryRootPath,
	'script',
	'node_modules',
	'.bin',
	npmBinName
	);
	return !external && fs.existsSync(localNpmBinPath)
	? localNpmBinPath
	: npmBinName;

installApm doesn't pass an argument, so !external will be true (and the only argument ever passed is ci anyway, which is expected to be false on local builds).

[aminya]

fs.existsSync(localNpmBinPath) will be false.

[Aerijo]

It might not be though, that's what makes it a race condition.

The first run is as per normal: first scripts dependencies, then apm install. It indicates the apm install is using the local npm. The second (after deleting scripts/node_modules) skips the dependencies and does apm install straight away. It indicates the apm install is using the environment npm.

The second scenario may occur if both are run in parallel, as it depends on if npm is installed before the apm install does the file exists check. In fact, it's highly likely to occur.

Now I've made it clear I don't care for having 3 different npm installs in the first place, but this PR would still change existing behaviour into a theoretically unpredictable way.

[aminya]

We can't control the local condition currently. For example, you can try to bootstrap using Node 14, and it will fail. In a local condition, you are expected to use the recommended versions.

We have a whole issue on this. In the near future, I will write some PowerShell scripts to fix this issue.
atom-community#111

To have repeatable builds, you should first run ./script/clean and then pass --ci to bootstrap which cleans the directory first and installs based on the lock files.

[sadick254]

Thanks for the PR!

That would definitely be a useful addition to our build process. However, it is not something that is within the scope that we have envisioned for Atom. We are not planning to make any major improvements to our build process for the time being. We are focused on fixing user-facing bugs. For that reason, I'm going to close this.

Thanks again for contributing to Atom 👍