1430825 - commit hooks should chunk files to lint

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[:Gijs (he/him)]

STR:

1. touch, say, 200 JS files, for instance by running an automated script that replaces stuff.
2. commit with the eslint hook

ER:
things work

AR:
WindowsError [Error 206] The filename or extension is too long

Comment 1

[Andrew Halberstadt [:ahal]]

Thanks for filing. Mozlint currently creates one process per linter and puts them onto a job queue. We could say each job should have a max of 10 paths (or other arbitrary number) and chunk by the number of specified paths. This would have the side benefit of running more things in parallel.

Comment 2

[:Gijs (he/him)]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> Thanks for filing. Mozlint currently creates one process per linter and puts
> them onto a job queue. We could say each job should have a max of 10 paths
> (or other arbitrary number) and chunk by the number of specified paths. This
> would have the side benefit of running more things in parallel.

Sounds good to me! :-)

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1430825 - [mozlint] Split work up by paths instead of by linters,

The initial motivation for this patch, was to prevent command lines that are
too long on Windows. To that end, there is a cap to the number of paths that
can be run per job. For now that cap is set to 50. This will allow for an
average path length of 160 characters, which should be sufficient with room to
spare.

But another big benefit of this patch is that we are running more things in
parallel. Previously, mozlint ran each linter in its own subprocess, but that's
it. If running eslint is 90% of the work, it'll still only get a single
process. This means we are wasting cores as soon as the other linters are
finished.

This patch chunks the number of specified paths such that there will be N*L
jobs where 'N' is the number of cores and 'L' is the number of linters.  This
means even when there's a dominant linter, we'll be making better use of our
resources. This isn't perfect of course, as some paths might contain a small
number of files, and some will contain a very large number of files.  But it's
a start

A limitation to this approach is that if there are fewer paths specified than
there are cores, we won't schedule enough jobs per linter to use those extra
cores. One idea might be to expand specified directories and individually list
all the paths under the directory. But this has some hairy edge cases that
would be tough to catch. Doing this in a non-hacky way would also require a
medium scale refactor.

So I propose further parallelization efforts be destined for follow-ups.

Review commit: https://reviewboard.mozilla.org/r/213804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213804/

Comment 4

[Andrew Halberstadt [:ahal]]

To reduce overhead, the attached patch tries to chunk paths into the number of cores the user has. However, if a specified maximum is reached (in this patch 50), it'll spin up extra jobs to prevent a command line that's too long. With 50 paths, that will allow an average of 160 characters/path before the limit is reached. I *think* that should be good enough. We could always lower the limit, or make sure we're passing in relative paths from topsrcdir if it's still a problem.

This also has some nice per wins. I ran:
$ ./mach lint toolkit/components/* browser/base/content/* dom/* layout/* js/src/* devtools/client/*

with patch: 46.694s
without patch: 125.52s

There will only be a perf benefit if we pass in multiple paths. Passing in a single path will still run everything on a single core. I briefly looked into expanding directories to bump up the number of paths when this happens, but there are some edge cases that didn't seem to work out very well, so I'd like to save that idea for a follow-up.

Comment 5

[Mark Banner (:standard8)]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> There will only be a perf benefit if we pass in multiple paths. Passing in a
> single path will still run everything on a single core. I briefly looked
> into expanding directories to bump up the number of paths when this happens,
> but there are some edge cases that didn't seem to work out very well, so I'd
> like to save that idea for a follow-up.

Just to note, ESLint does have https://github.com/eslint/eslint/issues/3565 on file for multi-process capability. There's been some recent work, but no expected timescales at the moment.

I guess we'll still need to split stuff up on Windows due to the path lengths, but we might need to think about that at some stage if/when it does happen.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8943463 [details]
Bug 1430825 - [mozlint] Split work up by paths instead of by linters,

https://reviewboard.mozilla.org/r/213804/#review220192

Looks good. r=Standard8

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bb16f349a38
[mozlint] Split work up by paths instead of by linters, r=standard8

Comment 8

[Raul Gurzau (:RaulG)]

Backed out changeset 5bb16f349a38 (bug 1430825) for Windows build bustage on a CLOSED TREE

Backout: https://hg.mozilla.org/integration/autoland/rev/0a104615fc631d681453de9ed5dbab6ca51d4907

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5bb16f349a386e3035c6d46a474fdc4ac9de71ee

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=157779168&repo=autoland&lineNumber=39159

Comment 9

[Andrew Halberstadt [:ahal]]

Oops, forgot these tests were still run as part of the build on Windows so didn't include it in my try run.

Comment 10

[Andrew Halberstadt [:ahal]]

The tests are failing because pytest's monkeypatch fixture doesn't play nicely with multiprocessing on Windows. I'll see if I can figure out an easy workaround, but may just end up skipping the tests on Windows for now.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8943463 [details]
Bug 1430825 - [mozlint] Split work up by paths instead of by linters,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213804/diff/1-2/

Comment 12

[Andrew Halberstadt [:ahal]]

Couldn't see an obvious alternative to count the number of jobs, so just ended up skipping on Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2311eb56fb4955a6d3b2566f62fd67a08bdc10f1

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/740b561ff8be
[mozlint] Split work up by paths instead of by linters, r=standard8

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/740b561ff8be