[PATCH] D28420: [lit] Limit parallelism of sanitizer tests on Darwin

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Generally looks good to me, nitpicks below.

You may consider using the word "pool" instead of "parallelism_group" to match the terminology of the ninja builder.



================
Comment at: projects/compiler-rt/test/asan/lit.cfg:245-246
+
+if config.host_os == 'Darwin':
+  if config.target_arch in ["x86_64", "x86_64h"]:
+    config.parallelism_group = "darwin-64bit-sanitizer"
----------------
could be
```
if config.host_os == 'Darwin' and config.target_arch in ["x86_64", "x86_64h"]:
```


================
Comment at: projects/compiler-rt/unittests/lit.common.unit.cfg:39-41
+    if test.file_path.find("x86_64") != -1:
+      return "darwin-64bit-sanitizer"
+    return ""
----------------
`return "darwin-64bit-sanitizer" if "x86_64" in test.file_path else ""`


================
Comment at: utils/lit/lit/run.py:178
         self.tests = tests
+        self.parallelism_semaphores = ""
 
----------------
This should rather be `dict()` or left out


================
Comment at: utils/lit/lit/run.py:182
+        pg = test.config.parallelism_group
+        if callable(pg): pg = pg(test)
+
----------------
Supporting callbacks here feels overengineered, but if you have to...


================
Comment at: utils/lit/lit/run.py:185
         result = None
+        if pg: self.parallelism_semaphores[pg].acquire()
         start_time = time.time()
----------------
I would move the name lookup into the `try:` part, so that for an invalid name lit doesn't shutdown completely.


https://reviews.llvm.org/D28420