1521772 - Run clang-format in parallel

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

I'd like to make clang-format a first class citizen in my workflow by having mercurial commit hooks (or some form of hook) running clang-format for me. At the moment, it is really really slow, though; it takes around 10 seconds to run just for the js/src/wasm directory, even when it's already correctly formatted. My machine is quite powerful, with SSD disks and yadda yadda, so I'd expect this to take less time.

Sylvestre on IRC told me that we're not doing this in parallel (for instance, we could run N processes, where N is the number of CPUs on the machine or so).

Anything that would reduce the time to run clang-format in general would be really nice to have.

Comment 1

[Andrew Halberstadt [:ahal]]

See the discussion in this other bug. If we decide to go that route, we can likely close/dupe this to that bug.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: multiprocessing.patch

Some quick benchmark using the multiprocessing Python module:

• on js/src/wasm (52 files), run time went from 11 seconds to 5 on my machine (CPU usage around 300%, not great).
• on js/src (1086 files), run time went from 1 minute 40 seconds down to 11 seconds on my machine (CPU usage around 1400%, better but still not perfect considering I've got 28 cores so it could be up to 2800%).

What do you think of landing this until we get a better proper implementation that works for all the lint tools (as suggested in the other bug that i skimmed, so maybe i misunderstood it)?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 9044184 [details] [diff] [review]
multiprocessing.patch

Please publish it to phabricator

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1521772: Run clang-format in parallel; r?sylvestre

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b59977d42a4d
Run clang-format in parallel; r=sylvestre

Comment 6

[Cristina Coroiu [:ccoroiu]]

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

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 9044184 [details] [diff] [review]
multiprocessing.patch

># HG changeset patch
># User Benjamin Bouvier <[e-mail]>
># Date 1550238872 -3600
>#      Fri Feb 15 14:54:32 2019 +0100
># Node ID a19c68106ddcfb25db078729dd81a5a950a18deb
># Parent  c5a43a8dd8f29bfeaa0d5fceb9177e19d7fdc931
>Bug XXX: Run clang-format in parallel; r?
>
>diff --git a/python/mozbuild/mozbuild/mach_commands.py b/python/mozbuild/mozbuild/mach_commands.py
>--- a/python/mozbuild/mozbuild/mach_commands.py
>+++ b/python/mozbuild/mozbuild/mach_commands.py
>@@ -69,16 +69,26 @@ If you feel this message is not appropri
> please file a Firefox Build System :: General bug at
> https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox%20Build%20System&component=General
> and tell us about your machine and build configuration so we can adjust the
> warning heuristic.
> ===================
> '''
> 
> 
>+# Function used by clang-format to run it in parallel, according to the given
>+# arguments. Must be defined at the top-level so it can be used with
>+# multiprocessing.Pool.imap_unordered.
>+def run_one_batch(args):
>+    try:
>+        subprocess.check_output(args)
>+    except subprocess.CalledProcessError as e:
>+        return e
>+
>+
> class StoreDebugParamsAndWarnAction(argparse.Action):
>     def __call__(self, parser, namespace, values, option_string=None):
>         sys.stderr.write('The --debugparams argument is deprecated. Please ' +
>                          'use --debugger-args instead.\n\n')
>         setattr(namespace, self.dest, values)
> 
> 
> @CommandProvider
>@@ -2856,56 +2866,69 @@ class StaticAnalysis(MachCommandBase):
> 
>         path_list = self._generate_path_list(paths)
> 
>         if path_list == []:
>             return
> 
>         print("Processing %d file(s)..." % len(path_list))
> 
>-        batchsize = 200
>         if show:
>-            batchsize = 1
>-
>-        for i in range(0, len(path_list), batchsize):
>-            l = path_list[i: (i + batchsize)]
>-            if show:
>+            for i in range(0, len(path_list)):
>+                l = path_list[i: (i + 1)]
>+
>                 # Copy the files into a temp directory
>                 # and run clang-format on the temp directory
>                 # and show the diff
>                 original_path = l[0]
>                 local_path = ntpath.basename(original_path)
>                 target_file = os.path.join(tmpdir, local_path)
>                 faketmpdir = os.path.dirname(target_file)
>                 if not os.path.isdir(faketmpdir):
>                     os.makedirs(faketmpdir)
>                 shutil.copy(l[0], faketmpdir)
>                 l[0] = target_file
> 
>-            # Run clang-format on the list
>-            try:
>-                check_output(args + l)
>-            except CalledProcessError as e:
>-                # Something wrong happend
>-                print("clang-format: An error occured while running clang-format.")
>-                return e.returncode
>-
>-            if show:
>+                # Run clang-format on the list
>+                try:
>+                    check_output(args + l)
>+                except CalledProcessError as e:
>+                    # Something wrong happend
>+                    print("clang-format: An error occured while running clang-format.")
>+                    return e.returncode
>+
>                 # show the diff
>                 diff_command = ["diff", "-u", original_path, target_file]
>                 try:
>                     output = check_output(diff_command)
>                 except CalledProcessError as e:
>                     # diff -u returns 0 when no change
>                     # here, we expect changes. if we are here, this means that
>                     # there is a diff to show
>                     if e.output:
>                         print(e.output)
>-        if show:
>+
>             shutil.rmtree(tmpdir)
>+            return 0
>+
>+        import multiprocessing
>+        import math
>+
>+        cpu_count = multiprocessing.cpu_count()
>+        batchsize = int(math.ceil(float(len(path_list)) / cpu_count))
>+        batches = [args + path_list[i: (i + batchsize)] for i in range(0, len(path_list), batchsize)]
>+
>+        pool = multiprocessing.Pool(cpu_count)
>+
>+        error_code = None
>+        for result in pool.imap_unordered(run_one_batch, batches):
>+            if error_code is None and result is not None:
>+                print("clang-format: An error occured while running clang-format.")
>+                error_code = result.returncode
>+
>         return 0
> 
> @CommandProvider
> class Vendor(MachCommandBase):
>     """Vendor third-party dependencies into the source repository."""
> 
>     @Command('vendor', category='misc',
>              description='Vendor third-party dependencies into the source repository.')

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/121bbaec1b30
Backed out changeset b59977d42a4d requested by Andi. a=backout