Install files in parallel #1256
Conversation
src/vcpkg/commands.install.cpp
Outdated
| #ifdef _WIN32 | ||
| std::sort(std::execution::par_unseq, output.begin(), output.end()); | ||
| #else | ||
| std::sort(output.begin(), output.end()); | ||
| #endif |
There was a problem hiding this comment.
I'm open for suggestions on how to implement parallel sorting for all platforms
src/vcpkg/commands.install.cpp
Outdated
| fs.remove_all(target, IgnoreErrors{}); | ||
| fs.remove(target, IgnoreErrors{}); |
There was a problem hiding this comment.
There's no difference for regular files between remove_all and remove (provided that vcpkg's implementation is identical to the STL): https://en.cppreference.com/w/cpp/filesystem/remove
…/vcpkg-tool into parallel-file-install
|
@BillyONeal Unit tests on OSX failed here. It seems to work again but I can consistently reproduce the failure on my local machine: The test case |
|
Btw you can build the format target locally to format all code. |
|
Sorry! |
…/vcpkg-tool into parallel-file-install
There was a problem hiding this comment.
I'm in general happy with attempts to parallelize things but overall I think you need to be much more explicit in proving that everything you call in a parallel context is actually safe to do. Given that the first run of e2e tests experienced a failure and I found races in a cursory review makes me fear that unknown races remain without such a proof.
This means if you want to proceed here I think you need to show strong thread safety requirements of every API you're calling rather than taking large blocks of existing code and wrapping them in parallel constructs.
| } | ||
| } | ||
|
|
||
| fs.create_directories(listfile.parent_path(), VCPKG_LINE_INFO); |
There was a problem hiding this comment.
This races with other file operations you're doing.
| if (ec) | ||
| { | ||
| msg::println_error(msgInstallFailed, msg::path = target, msg::error_msg = ec.message()); | ||
| Debug::println("Install from packages to installed: Fallback to copy " |
There was a problem hiding this comment.
Debug::println is not thread safe.
There was a problem hiding this comment.
There is other parallelized code where this also happens: in cmd_execute_and_capture_output_parallel we call cmd_execute_and_stream_data which calls Debug::print().
I think the main issue with parallelizing here is printing. Basically, whenever the program can print anything, needs to be guarded by a mutex which is IMO not possible from the outside. Therefore, we need to make printing inherently thread safe. |
|
@BillyONeal now that #1323 landed, what needs to be done for this to land? I checked each function for thread safety. The following functions are not thread safe because they print to the console:
I'm not entirely sure how to use |
Depending on how many files to install, this change results in a >= 4x speed up. I only tested relatively small ports but I expect even higher speed ups for bigger ports.