potential multiprocessing performance improvement

[asottile]

I swear this isn't just an advertisement for my youtube video... but along the same lines of my youtube video multiprocessing: slow things first I believe there's some optimization potential to the scheduling of files for black

I initially thought about this a bit when receiving a report from freecad about slow execution of black in pre-commit.ci -- (sorta unrelated to this) after looking into it I noticed two significant improvements to their setup: (1) use the pre-compiled black (next pre-commit.ci autoupdate will do this fleetwide automatically!) and (2) there were a bunch of very large files which black was spending most of its time on

the second is interesting to me and where I reminded myself of potential optimizations! the tl;dr of the optimization is to attempt "slower" things first such that a scheduler can appropriately binpack smaller things later (imagine the worst case scenario where the last thing you schedule is the slowest one and you've significantly extended your execution time while having mostly idle cores)

I decided to try an experiment with black and see if applying a similar optimization could improve performance. my numbers below are on a VM with 12 cores against this version of freecad with this diff applied:

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 101f719e..8e277c8d 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -44,7 +44,6 @@ exclude: |
         src/Gui/3Dconnexion/navlib|
         src/Gui/QSint|
         src/Gui/Quarter|
-        src/Mod/Fem/femexamples|
         src/Mod/Import/App/SCL|
         src/Mod/Import/App/SCL_output|
         src/Mod/Mesh/App/TestData|
@@ -70,3 +69,13 @@ repos:
     rev: 719856d56a62953b8d2839fb9e851f25c3cfeef8  # frozen: v21.1.2
     hooks:
         -   id: clang-format
+-   repo: local
+    hooks:
+    - id: black-local
+      name: black
+      description: "Black: The uncompromising Python code formatter"
+      entry: black --line-length 100
+      language: system
+      minimum_pre_commit_version: 2.9.2
+      require_serial: true
+      types_or: [python, pyi]

and I wrote a little shell script to run this:

#!/usr/bin/env bash
set -euxo pipefail

pip uninstall -yqq black
pip install -qq /tmp/black --no-deps

rm -rf ~/.cache/black
pre-commit run black-local --all-files --verbose

(note this uses non-precompiled black -- I suspect the numbers are similar but maybe a little less pronounced on a precompiled black)

(note also I haven't intentionally chosen a terrible set of files -- if I wanted a worst case scenario I'd probably delete every file after the ones in src/Mod/Fem/femexamples such that it schedules those last)

first a little baseline -- I ran this a few times and this particular run seemed representative (again, not very scientific performance benchmark but I was mostly just exploring the idea)

$ bash t.sh 
+ pip uninstall -yqq black
+ pip install -qq /tmp/black --no-deps
+ rm -rf /home/asottile/.cache/black
+ pre-commit run black-local --all-files --verbose
black....................................................................Passed
- hook id: black-local
- duration: 32.82s

All done! ✨ 🍰 ✨
1002 files left unchanged.

after applying this patch to black (which naively assumes that larger files will be slower -- I think this is a pretty good assumption):

diff --git a/src/black/concurrency.py b/src/black/concurrency.py
index f6a2b8a..fd8db5f 100644
--- a/src/black/concurrency.py
+++ b/src/black/concurrency.py
@@ -164,7 +164,7 @@ async def schedule_formatting(
                 executor, format_file_in_place, src, fast, mode, write_back, lock
             )
         ): src
-        for src in sorted(sources)
+        for src in sorted(sources, key=lambda f: (-f.stat().st_size, f))
     }
     pending = tasks.keys()
     try:

I get a pretty significant improvement! (again: anecdotal -- though a few runs were similar in timing)

$ bash t.sh 
+ pip uninstall -yqq black
+ pip install -qq /tmp/black --no-deps
+ rm -rf /home/asottile/.cache/black
+ pre-commit run black-local --all-files --verbose
black....................................................................Passed
- hook id: black-local
- duration: 26.94s

All done! ✨ 🍰 ✨
1002 files left unchanged.

so maybe there's some potential here?

[hauntsaninja]

Interesting idea, thank you! We could also reuse the stat's from the cache checking logic for this