GeezerVison is showing github.com — Concurrent execution results in uneven work per thread · Issue #861 · pre-commit/pre-commit
Concurrent execution results in uneven work per thread #861DescriptionI'm running pre-commit from current master to test the concurrency feature introduced with #851. While it in general seems to work, work is distributed pretty uneven. One hook we run is prospector which is nice for testing, because it takes a relatively long time and it prints the time taken in its output.
Running pre-commit run -a --verbose prospector | grep "Time Taken" on a medium sized project (~100 Python files) results in the following distribution of work to the available 4 logical CPU cores:
Time Taken: 17.10 seconds
Time Taken: 8.70 seconds
Time Taken: 18.68 seconds
Time Taken: 108.02 seconds
Especially compared to running it with concurrency disabled (using PRE_COMMIT_NO_CONCURRENCY), it's pretty obvious that concurrency doesn't provide any real benefit here:
Time Taken: 116.95 seconds
I'd be happy to help debugging this further. Just tell me what other information you need. 🙂 Activityasottile commented on Nov 7, 2018 chriskuehl commented on Nov 7, 2018 It would be interesting to look at how it's partitioning the files. I'd guess it's either some files taking way more time than others to process, or maybe an issue with max arg length forcing it to emit another partition (though this seems unlikely given four partitions). Dunedan commented on Nov 7, 2018 @asottile Unfortunately it's a private repo, so no poking for you.
@chriskuehl If you can point me to how to get more insight into the partitioning, I can check if I can figure out what the issue could be. Dunedan commented on Nov 8, 2018 I wrote a little script-hook to print out the file names and sum up the number of lines in these files. That results in the following distribution of lines:
From that it's apparent that the distribution of lines is uneven (although not as uneven as I'd have expected from the time taken by prospector, so either prospector doesn't scale linearly or there is more to it than the number of lines). The uneven distribution is caused by pre-commit using an ordered list of files for partitioning. So e.g. one process might get just files with application logic and another just files with tests, which differ in their average number of lines. I believe files with a similar number of lines being grouped together would be a common problem for other users as well.
I then tried to just shuffle the order of files before partitioning and it did indeed help and times went down quite a bit. These are the times for three different invocations (remember the order is random and differs per invocation, that's why the times differ that much):
Time Taken: 54.20 seconds
Time Taken: 16.44 seconds
Time Taken: 15.31 seconds
Time Taken: 23.66 seconds
Time Taken: 52.63 seconds
Time Taken: 20.48 seconds
Time Taken: 32.03 seconds
Time Taken: 24.21 seconds
Time Taken: 25.09 seconds
Time Taken: 48.53 seconds
Time Taken: 26.13 seconds
Time Taken: 23.39 seconds
Still not perfect, but already quite an improvement. Would such a shuffling (maybe in a deterministic fashion) before partitioning (and maybe re-ordering per partition) be an option? asottile commented on Nov 8, 2018 I think we want to keep with as deterministic as possible. It was changed in #767 chriskuehl commented on Nov 8, 2018 I thought about shuffling too but rejected it for the same reason.
I suppose we could shuffle it deterministically (e.g. use a fixed random seed so that the same set of files always partition the same way), but that could still be confusing. asottile commented on Nov 8, 2018 deterministic shuffling I guess wouldn't be the worst. though I can see it being confusing when adding / removing a file with mypy 🤷♂️ Dunedan commented on Nov 12, 2018 Another idea would be to increase the number of partitions past the number of available CPU cores. I tried doubling the number of partitions and with that I see some improvement, but not as much as I'd expect, apparently because the files which take to most time still end up in the same partition:
Mo 12. Nov 16:01:38 CET 2018
Time Taken: 6.82 seconds
Time Taken: 16.71 seconds
Time Taken: 5.65 seconds
Time Taken: 5.16 seconds
Time Taken: 17.90 seconds
Time Taken: 8.67 seconds
Time Taken: 71.29 seconds
Time Taken: 10.88 seconds
Mo 12. Nov 16:03:00 CET 2018
chriskuehl commented on Nov 12, 2018 I think the problem with intentionally having more partitions than the CPU count is that we'll have to pay any fixed startup costs (e.g. initializing the hook) multiple times in serial. For most hooks it's probably not a big deal, but could be bad for hooks that are slow to start. Dunedan commented on Nov 13, 2018
I think the problem with intentionally having more partitions than the CPU count is that we'll have to pay any fixed startup costs (e.g. initializing the hook) multiple times in serial. For most hooks it's probably not a big deal, but could be bad for hooks that are slow to start.
Of course there are always trade-offs. So what's more common: Hooks with an initialization time, which is so long compared to their run time that initializing them multiple times would cause a significant performance decrease or hooks which would benefit from more partitions? I don't know the answer and I guess that you don't either.
What about gathering data to tune the options to fit best for as many use cases as possible? As we know there are cases which don't really benefit from the current concurrency settings.
What about taking an exemplary set of files with an exemplary set of pre-commit hooks and benchmark them on different machines to get proper data? For Pylint for example there is a special set of data for benchmarking: https://github.com/brycepg/pylint-corpus. Aside from such a set of data, pre-commit would need to offer the ability to log more details about the concurrency, without the need for manual patching. We could then use that data to tune options like the number of partitions, number of threads, shuffling of files before partitioning, etc. to achieve the best possible result for as many use cases as possible. asottile commented on Nov 13, 2018 The most common cases are probably python hooks. On Linux this is bare minimum 100ms startup cost per process (no imports) but often more (2-300 ms). On windows the problem is much much worse often taking 3-500 ms. I think we want to minimize startup as much as possible.
If deterministic shuffling solves this, let's do it. asottile commented on Nov 19, 2018 I kinda think this is a blocker for the next release -- I think deterministic shuffling is probably the "best" we can do here, should I/we/you/someone implement that? chriskuehl commented on Nov 19, 2018 @asottile I can give this a try later this afternoon chriskuehl commented on Nov 20, 2018 Deterministic shuffling seems to do about as good of a job at balancing as could be expected. I wrote a silly bash hook which just prints the output of wc -l over the lines passed to it. If I run the hook over a large Python project (I used https://github.com/translate/pootle) on a machine with 12 cores, it looks like this:
(py27) ckuehl@ithuriel:~/proj/pootle$ pre-commit run --all-files wc -v
[wc] wc..................................................................Passed
hookid: wc
8807
12158
5213
129052
273175
254326
11497
10273
22560
2905
239681
30047
10666
Not very balanced. With shuffling it looks like:
(py27) ckuehl@ithuriel:~/proj/pootle$ pre-commit run --all-files wc -v
[wc] wc..................................................................Passed
hookid: wc
112387
76954
76587
79163
78836
118854
85061
57289
78456
75089
99813
80119
Obviously the actual benefit is going to depend on the specific hook and a dice roll, but in most cases this will probably improve things about as well as we can without actively trying to balance line lengths between the partitions.
Another benefit of shuffling is that it will probably tend to fill the partitions to the maximum argument length more reliably, since you won't have long stretches of very long file names (due to a bunch of deeply nested files). Reacted by anthony sottile Reacted by anthony sottile Dunedan commented on Nov 20, 2018 Thanks a lot for addressing this. 👍
Unfortunately the seed for shuffling chosen in #874 makes it even worse for my case, as the number of lines to be processed per thread becomes even more uneven. That's of course a problem for my specific case, but I guess it shows that shuffling isn't a perfect solution either. 😞 chriskuehl commented on Nov 20, 2018 @Dunedan that's unfortunate :(
Just curious, if you manually tweak the random seed (you should just be able to change it arbitrarily to any numbers), are there scenarios where it's better? Wondering if it's just an unlucky shuffle or a more fundamental problem.
Kind of weird that you're hitting this with only 100 Python files -- I wonder if this hook is just so slow for a few of your ~100 files that this is especially hard to avoid. Dunedan commented on Nov 21, 2018 It's definitely an unlucky shuffle. If I e.g. just adjust the seed by ±1 I get a really good benefit from concurrent execution.
The project I'm talking about is in active development so the introduction/removal of files in future might change the resulting partitions resulting in more beneficial results.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|