Parallelize enforcement of clang-format #163

BillyONeal · 2019-10-05T04:08:50Z

A component we needed as part of our minimal test harness was some ability to capture command line output from multiple subprocesses in parallel. There's a useful program we can build out of that without having test discovery working yet, just a little helper to run a command against every file in tree(s) in parallel, protected by NT Jobs. This helps flush out bugs in the scheduling machinery without also needing to worry about bugs in the test discovery/execution, as well as just producing a useful program we might use elsewhere.

I expect this to actually be most of the code for the test harness since our tests consist of "grep this tree for .cpp files, compile them with compiler switch cocktails, and ensure the output program works".

Time improvement comparison:
Most recent successful build https://dev.azure.com/vclibs/STL/_build/results?buildId=629 spends 37s enforcing clang-format.
First successful build of this PR https://dev.azure.com/vclibs/STL/_build/results?buildId=635 spends 7s building the support tools and 8s enforcing clang-format (60% less time than serial even with building parallelize.exe overhead)

Add a "tools" directory for test/build support tools. Add jobify.exe from the msvc repo. Extract parts of jobify into stljobs.h, and author wrappers for other test support. Add parallelize.exe which runs a command in parallel over all inputs in a directory.

azure-devops/run_build.yml

azure-devops/enforce-clang-format.cmd

azure-devops/run_build.yml

tools/parallelize/parallelize.cpp

GorNishanov · 2019-10-08T03:52:58Z

tools/inc/stljobs.h

+        return *this;
+    }
+
+    handle& operator=(const HANDLE hNew) & noexcept {


I would rename this to "attach" by analogy with "detach" that you already have. Since, this is "adopt" ownership operation, I think it should be more conspicuous than plain assignment.

GorNishanov · 2019-10-08T03:55:00Z

tools/inc/stljobs.h

+        return std::exchange(hImpl, EmptyPolicy::Empty);
+    }
+
+    [[nodiscard]] HANDLE* out() noexcept {


I would remove this API completely. It is dangerous, since, the c-APIs that you pass this handle out are unaware of any invariants this class might enforce. [I believe later in the code review we will discover the pitfall with this API, stay tuned]

GorNishanov · 2019-10-08T03:59:18Z

tools/inc/stljobs.h

+        inheritSa.nLength              = sizeof(inheritSa);
+        inheritSa.lpSecurityDescriptor = nullptr;
+        inheritSa.bInheritHandle       = TRUE;
+        HANDLE read;


I don't think read can be handle wrapper here and neither can be hDevNull (remember the warning about having .out()] API earlier?

If CreatePipe failes, it can leave arbitrary garbage in its out parameters, the wrapper will try to close it and can lead to sadness. See MSDN for CreatePipe:

If CreatePipe fails, the contents of the output parameters are indeterminate.
No assumptions should be made about their contents in this event.

GorNishanov · 2019-10-08T03:59:52Z

tools/inc/stljobs.h

+        inheritSa.lpSecurityDescriptor = nullptr;
+        inheritSa.bInheritHandle       = TRUE;
+        HANDLE read;
+        if (!CreatePipe(&read, hDevNull.out(), &inheritSa, 0)) {


Replace hDevNull with a raw handle. If the API call was successful, let hDevNull adopt the resulting raw handle

GorNishanov · 2019-10-08T04:01:47Z

tools/inc/stljobs.h

+    tp_io() = default;
+
+    explicit tp_io(handle<invalid_handle_value_policy>&& fileHandle_, PTP_WIN32_IO_CALLBACK callback, void* const pv,
+        const PTP_CALLBACK_ENVIRON pcbe)


const PTP_CALLBACK_ENVIRON pcbe = nullptr. Since we are going to use the default environment, making the parameter optional will remove the clutter at the point of use (and attempting to look up what is that nullptr last parameter do)

tools/inc/stljobs.h

GorNishanov · 2019-10-08T04:11:49Z

tools/inc/stljobs.h

+class environment_block {
+public:
+    environment_block() {
+        default_block defaultBlock;


It would be helpful to have a comment here clarifying that we are trying to find the size of the environment block which is a zero separated sequence of strings ending with two zeroes. Hence, needing to count first before assigning it to std::string.

tools/inc/stljobs.h

GorNishanov · 2019-10-08T04:30:06Z

tools/inc/stljobs.h

+        case ERROR_OPERATION_ABORTED:
+            break;
+        default:
+            api_failure("StartThreadpoolIo callback", ioResult);


Nitpick: Failure here indicates a completion of ReadFile API with error, not a failure of StartThreadpoolIo. A future reader (of the code or logs) may be confused about which API failed.

Also, we are throwing here on a threadpool thread that does not have a try-catch and won't nice print anything. I am wondering, if we would want to log something in this case so that we are not silent when it happens. For example, we can log in the api_failure itself.

GorNishanov · 2019-10-08T04:38:12Z

tools/jobify/jobify.cpp

+
+#include <Windows.h>
+
+wchar_t* get_subcommand() {


I would put a comment clarifying what are we hunting for and an example string it works upon.
Also, I would explain that it returns a pointer to a non-const wchar_t due to peculiarities of the CreateProcess API which insists on scribbling on its input arguments. :-)

The non-const wchar_t are because that's what GetCommandLineW returns, not any particularity of CreateProcess (although working with CreateProcess is a nice side benefit of that)

tools/inc/stljobs.h

StephanTLavavej · 2019-10-08T05:12:00Z

tools/inc/stljobs.h

-    [[nodiscard]] HANDLE detach() noexcept {
-        return std::exchange(hImpl, EmptyPolicy::Empty);
+    void attach(const HANDLE newHandle) & noexcept {
+        handle moved{newHandle};


moved seems like the wrong name here; this isn’t a move assign.

tools/inc/stljobs.h

GorNishanov

LGTM!

StephanTLavavej · 2019-10-08T20:26:06Z

tools/inc/stljobs.h

+        }
+    }
+
+    [[nodiscard]] LPPROC_THREAD_ATTRIBUTE_LIST get() const {


StephanTLavavej · 2019-10-08T20:27:07Z

tools/inc/stljobs.h

+    output_collecting_pipe& operator=(const output_collecting_pipe&) = delete;
+
+    void start() {
+        [[maybe_unused]] const auto oldRunning = running.exchange(true);


(Repeating previous comment) Should this be bool instead of auto? Ok to leave as-is if you want this.

StephanTLavavej · 2019-10-08T20:29:12Z

tools/parallelize/parallelize.cpp

+            parent->update_display();
+        }
+
+        void schedule(size_t command) {


const size_t command?

Parallelize enforcement of clang-format (microsoft#163)

ariccio · 2020-09-25T04:30:03Z

tools/inc/stljobs.h

+    throw api_exception{api, lastError};
+}
+
+void close_handle(const HANDLE toClose) noexcept {


Can I chime in like a whole year later and say that a neat trick I discovered years ago is that by marking HANDLES like this parameter with _Post_ptr_invalid_ you get diagnostics from code analysis if you reuse handles that are closed

BillyONeal added 2 commits October 4, 2019 20:58

Teach Azure DevOps to enforce clang-format in parallel.

6754cf0

BillyONeal requested a review from a team as a code owner October 5, 2019 04:08

BillyONeal requested a review from GorNishanov October 5, 2019 04:10

Attempt to make Pipelines happy, 1 of N

64244f1

BillyONeal commented Oct 5, 2019

View reviewed changes

azure-devops/run_build.yml Outdated Show resolved Hide resolved

BillyONeal added 4 commits October 4, 2019 21:23

Attempt to make Pipelines happy, 2 of N

75ba206

Attempt to make Pipelines happy, 3 of N

bccdb05

Attempt to make Pipelines happy, 4 of N

78f3f01

Attempt to make Pipelines happy, 5 of N

7724277

BillyONeal commented Oct 5, 2019

View reviewed changes

azure-devops/enforce-clang-format.cmd Outdated Show resolved Hide resolved

Enforce clang-format in the tools directory.

8ee38e0

StephanTLavavej requested changes Oct 8, 2019

View reviewed changes

Use CMAKE_CXX_STANDARD and /permissive-.

2bb38d5

GorNishanov reviewed Oct 8, 2019

View reviewed changes

BillyONeal added 3 commits October 7, 2019 21:31

Fix a whole boatload of Stephan CR feedback.

350611f

Print clang-format version.

613883b

Back out attempting to avoid building jobify.

5ae52d9

GorNishanov reviewed Oct 8, 2019

View reviewed changes

BillyONeal added 2 commits October 7, 2019 21:49

Fix assert and Gor PR feedback.

07544ed

More Gor PR feedback.

8073b04

BillyONeal requested review from GorNishanov and StephanTLavavej October 8, 2019 04:54

Add get_subcommand comment.

13d985a

StephanTLavavej reviewed Oct 8, 2019

View reviewed changes

StephanTLavavej approved these changes Oct 8, 2019

View reviewed changes

tools/inc/stljobs.h Show resolved Hide resolved

GorNishanov approved these changes Oct 8, 2019

View reviewed changes

StephanTLavavej approved these changes Oct 8, 2019

View reviewed changes

More CR feedback.

598768d

BillyONeal requested a review from StephanTLavavej October 8, 2019 23:49

StephanTLavavej approved these changes Oct 9, 2019

View reviewed changes

BillyONeal merged commit 9b2207c into microsoft:master Oct 9, 2019

BillyONeal deleted the testing branch October 9, 2019 00:12

fengjixuchui added a commit to fengjixuchui/STL that referenced this pull request Oct 9, 2019

Merge pull request #3 from microsoft/master

de74497

Parallelize enforcement of clang-format (microsoft#163)

StephanTLavavej mentioned this pull request Nov 5, 2019

Make enforce-clang-format.cmd also run git diff. #255

Merged

4 tasks

ariccio reviewed Sep 25, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parallelize enforcement of clang-format #163

Parallelize enforcement of clang-format #163

BillyONeal commented Oct 5, 2019 •

edited

Loading

GorNishanov Oct 8, 2019

GorNishanov Oct 8, 2019

GorNishanov Oct 8, 2019

GorNishanov Oct 8, 2019

GorNishanov Oct 8, 2019

GorNishanov Oct 8, 2019

GorNishanov Oct 8, 2019

GorNishanov Oct 8, 2019

BillyONeal Oct 8, 2019

StephanTLavavej Oct 8, 2019

GorNishanov left a comment

StephanTLavavej Oct 8, 2019

StephanTLavavej Oct 8, 2019

StephanTLavavej Oct 8, 2019

ariccio Sep 25, 2020


		#include <Windows.h>

		wchar_t* get_subcommand() {

Parallelize enforcement of clang-format #163

Parallelize enforcement of clang-format #163

Conversation

BillyONeal commented Oct 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GorNishanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Oct 5, 2019 •

edited

Loading