Closed Bug 1430825 Opened 4 years ago Closed 3 years ago

commit hooks should chunk files to lint

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Gijs, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Blocks: 1427845
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.
(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! :-)
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
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.
(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 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
Attachment #8943463 - Flags: review?(standard8) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5bb16f349a38
[mozlint] Split work up by paths instead of by linters, r=standard8
Oops, forgot these tests were still run as part of the build on Windows so didn't include it in my try run.
Flags: needinfo?(ahalberstadt)
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.
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
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/740b561ff8be
[mozlint] Split work up by paths instead of by linters, r=standard8
https://hg.mozilla.org/mozilla-central/rev/740b561ff8be
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.