Run clang-format in parallel

RESOLVED FIXED in Firefox 67

Status

enhancement
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

(Blocks 1 bug)

Trunk
mozilla67

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 months ago

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.

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

See Also: → 1511122
(Assignee)

Comment 2

3 months ago
Posted patch multiprocessing.patch (obsolete) — Splinter Review

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)?

Attachment #9044184 - Flags: feedback?(ahal)
Comment on attachment 9044184 [details] [diff] [review]
multiprocessing.patch

Please publish it to phabricator
Assignee: nobody → bbouvier
Severity: normal → enhancement

Comment 5

3 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b59977d42a4d
Run clang-format in parallel; r=sylvestre

Comment 6

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Comment on attachment 9044184 [details] [diff] [review]
multiprocessing.patch

># HG changeset patch
># User Benjamin Bouvier <benj@benj.me>
># 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.')
Attachment #9044184 - Attachment is obsolete: true
Attachment #9044184 - Flags: feedback?(ahal)

Comment 8

3 months ago
Backout by dvarga@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/121bbaec1b30
Backed out changeset b59977d42a4d requested by Andi. a=backout
Summary: Can we run clang-format in parallel? → Run clang-format in parallel
You need to log in before you can comment on or make changes to this bug.