1683392 - High CPU load for "mach puppeteer-test" since using "npm ci"

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now each call to mach puppeteer-test will install puppeteer. This causes lots of CPU usage for both nodejs and especially watchman. The latter runs even a minute after the mach command finished with more than 100% cpu load on my machine.

So we should only install puppeteer when it hasn't been done before, or if files for Puppeteer have been modified since the last time it got installed.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The problem here has actually been caused by the changes on bug 1670286, which changed the npm install step to npm ci.

Here some timings that do not include the watchman load afterward:
npm ci: 66.26s user 16.74s system 153% cpu 53.982 tota
npm install: 46.21s user 4.49s system 146% cpu 34.507 total

By using npm ci all the installed node modules are getting removed and re-installed. That seems to be unnecessary for doing it for every mach puppteer-test call. Maybe we could do:

1. If node modules haven't been installed yet, run npm ci.
2. If pinned modules have been changed, run npm ci.
3. If options above don't apply run npm install.

Or would install not take care of the pinned versions? Maja, maybe you can still help us here?

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I see a spike from watchman with npm ci, too, likely due to the clobber of node_modules, although the spike isn't as noticeable on my machine.

Without watchman, npm ci should actually be faster than npm install because it skips some steps. See https://docs.npmjs.com/cli/v6/commands/npm-ci#description

I think you should probably configure your watchman to ignore changes in node_modules before changing anything in the mach command.

Or you could consider using npm ci in our CI only. I think there's a TASKCLUSTER environment variable that you could check to determine whether the command is running in automation?

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #1)

1. If node modules haven't been installed yet, run npm ci.
2. If pinned modules have been changed, run npm ci.

npm ci exits with an error if the dependencies list in the package-lock.file don't match what gets generated from package.json. When package.json changes, you need to run npm install (and this is done automatically when running the vendoring mach command)

Or would install not take care of the pinned versions? Maja, maybe you can still help us here?

npm install writes a new package-lock.json file, which we need to do only when we sync with upstream.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mike, do we have any rules in where required npm packages need to be installed when running tests via mach? Right now all the packages for the Puppeteer jobs end-up in the source tree, and removing and reinstalling all of these dependencies cause a high CPU load for watchman for me. So I wonder if those packages should better be installed in the obj dir. If that's the preferred location we might also have to instruct the TypeScript compilation step to output the content there.

Comment 4

[Mike Hommey [:glandium]]

302 dmose

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

@Henrik: there are no specific rules about where node_modules get installed at this point. In some ideal world, they'd get installed in the objdir, but I'm not aware of any code that currently works that way. I suspect supporting that would be a bunch of work, but I don't know for sure.

Comment 2 proposes:

Or you could consider using npm ci in our CI only. I think there's a TASKCLUSTER environment variable that you could check to determine whether the command is running in automation?

This seems like The Right Thing, and it also sounds like it should be easy to implement. Why not just do this?

Comment 6

[Mark Banner (:standard8)]

I guess I'm needinfo'ed for ideas. On the ESLint front, we package node_modules into a zip file and upload that to the build infrastructure. It then gets included in the docker images, and used as necessary. This also means the builders have a defined version available and is reproducible, it also means that we're not dependent on the nodejs infrastructure (though it doesn't normally cause a problem).

We are looking to move the top level node_modules into the source tree, however that vendoring work is still in progress as there's various policies to be defined around it.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks for the feedback Mark! So we actually want a quick fix here, and using the build infra for that seems to be a bit of work involved. As such I would stick with keeping the node modules where they are right now and use an environment variable.

I discussed that with Ben Hearsum on Matrix, and as result we should better go with our own environment variable set in the Taskcluster configuration: https://searchfox.org/mozilla-central/source/taskcluster/ci/source-test/remote.yml

Reason is that we do not want to depend on a variable that might change at any time, and would cause side-effects.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually I will make use of a --ci command line argument, which will be better observable when running mach puppeteer-test --help.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1683392 - [puppeteer] Use "npm ci" only when tests are run on TaskCluster.

This patch adds a new "--ci" argument for the
"mach puppeteer-test" command. As such it can
also be used locally to simulate a test job in CI.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2f8cb04ce26
[puppeteer] Use "npm ci" only when tests are run on TaskCluster. r=remote-protocol-reviewers,jdescottes

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d2f8cb04ce26