Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binary cache: async push_success #908

Draft
wants to merge 60 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
95f0438
Binary cache: async push_success
autoantwort Feb 15, 2023
9d999d8
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Feb 28, 2023
163d9cd
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 2, 2023
2a54205
Apply suggestions from code review
autoantwort Mar 2, 2023
0912655
Adapt code review
autoantwort Mar 2, 2023
5d7288c
Update src/vcpkg/binarycaching.cpp
autoantwort Mar 2, 2023
10189ac
Adapt code review
autoantwort Mar 2, 2023
2567607
Remove unnecessary actions_to_push_notifier.notify_all()
autoantwort Mar 2, 2023
ecdd000
Prevent deadlock and don't be on the crtl+c path
autoantwort Mar 2, 2023
8e7ae61
Add and use BGMessageSink to print IBinaryProvider::push_success mess…
autoantwort Mar 3, 2023
850d7c9
Restore old upload message
autoantwort Mar 3, 2023
548be38
Don't join yourself
autoantwort Mar 4, 2023
6dbbf06
Print messages about remaining packages to upload
autoantwort Mar 4, 2023
74b86fd
Localization
autoantwort Mar 5, 2023
5171d3e
Improve messages
autoantwort Mar 5, 2023
d69ed8f
No singleton and explicit calls to wait_for_async_complete()
autoantwort Mar 5, 2023
2df42d5
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 8, 2023
5f1786e
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 10, 2023
93303c3
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 16, 2023
8a26c8b
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 19, 2023
aa7e52f
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Mar 22, 2023
d46a4d6
Apply code review
autoantwort Mar 22, 2023
5e51718
Trigger Build
autoantwort Mar 22, 2023
a9ac558
No rename dance
autoantwort Mar 22, 2023
4faf674
Print upload to provider only once and not once per provider
autoantwort Mar 22, 2023
b9be8c6
Fix tests
autoantwort Mar 22, 2023
78ca081
Don't create unnecessary strings
autoantwort Mar 31, 2023
579bfa9
Rename to m_published_lock
autoantwort Mar 31, 2023
103968e
BinaryPackageInformation use Optional and make BinaryProviderPushRequ…
autoantwort Mar 31, 2023
dd32416
Merge branch 'main' into feature/async-binary-cache-push-success and …
autoantwort May 31, 2023
b666f94
Add missing files
autoantwort May 31, 2023
15bb503
Add missing includes
autoantwort May 31, 2023
d995bfd
Make BianryCache a unique_ptr
autoantwort May 31, 2023
24cd026
Reduce changes
autoantwort May 31, 2023
92fc76b
Fix output
autoantwort May 31, 2023
3527227
Fix bug
autoantwort May 31, 2023
48305b3
Format
autoantwort May 31, 2023
27fa076
Use lock_guard
autoantwort May 31, 2023
bcd459a
Revert "Use lock_guard"
autoantwort May 31, 2023
f958d36
Use enum
autoantwort May 31, 2023
7a24007
BGMessageSink::print_published apply code review
autoantwort May 31, 2023
50114f9
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Jun 14, 2023
ca5f2b1
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Aug 19, 2023
eccd9ee
Fix typo
autoantwort Aug 24, 2023
e7837e0
Fix typo in file name
autoantwort Aug 24, 2023
969e7fc
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Aug 24, 2023
2d5586f
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Oct 11, 2023
809d0b6
Renamings
autoantwort Oct 11, 2023
455e29b
format
autoantwort Oct 12, 2023
03fdfea
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Nov 4, 2023
f4bad8c
BinaryCache and std::unique_ptr
autoantwort Nov 4, 2023
26bbbd5
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Nov 12, 2023
814e434
BinaryCache: save data in std::unique_ptr so that the object can be m…
autoantwort Nov 14, 2023
290e586
fix
autoantwort Nov 14, 2023
3cc3378
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Dec 13, 2023
978ceae
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Nov 18, 2024
47b56ce
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Dec 25, 2024
061e6e8
Merge branch 'main' into feature/async-binary-cache-push-success
autoantwort Jan 14, 2025
050c51f
Merge remote-tracking branch 'origin/main' into feature/async-binary-…
BillyONeal Jan 15, 2025
8182732
Merge remote-tracking branch 'origin/main' into feature/async-binary-…
BillyONeal Jan 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions include/vcpkg/base/batch-queue.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#pragma once

#include <condition_variable>
#include <mutex>
#include <vector>

template<class T>
class BatchQueue
{
public:
template<class... Args>
void push(Args&&... args)
{
forward.emplace_back(std::forward<Args>(args)...);
}

bool empty() const { return forward.empty(); }

void pop(std::vector<T>& out)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this thing being called queue given that this is how it works. Given that we expect this to be a multi producer single consumer queue, can we instead put the vector inside and note that only one thread may call pop but any number of threads may call push? That would also resolve the criticism over separate tracking atomics below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BatchQueue alone is not thread safe.

Given that we expect this to be a multi producer single consumer queue, can we instead put the vector inside and note that only one thread may call pop but any number of threads may call push?

I don't get what you have in mind here 😅

I don't like this thing being called queue given that this is how it works.

Do you have an idea for a better name? :)

{
out.clear();
swap(out, forward);
}

private:
std::vector<T> forward;
};

template<class WorkItem>
struct BGThreadBatchQueue
{
template<class... Args>
void push(Args&&... args)
{
std::lock_guard<std::mutex> lock(m_mtx);
m_tasks.push(std::forward<Args>(args)...);
m_cv.notify_all();
}

void wait_for_items(std::vector<WorkItem>& out)
{
std::unique_lock<std::mutex> lock(m_mtx);
m_cv.wait(lock, [this]() { return !m_tasks.empty() || !m_running; });
m_tasks.pop(out);
}

void stop()
{
std::lock_guard<std::mutex> lock(m_mtx);
m_running = false;
m_cv.notify_all();
}

bool stopped()
{
std::lock_guard<std::mutex> lock(m_mtx);
return !m_running;
}

private:
std::mutex m_mtx;
std::condition_variable m_cv;
BatchQueue<WorkItem> m_tasks;
bool m_running = true;
};
1 change: 1 addition & 0 deletions include/vcpkg/base/fwd/message_sinks.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ namespace vcpkg

struct FileSink;
struct TeeSink;
struct BGMessageSink;
}
1 change: 1 addition & 0 deletions include/vcpkg/base/message-data.inc.h
Original file line number Diff line number Diff line change
Expand Up @@ -3263,6 +3263,7 @@ DECLARE_MESSAGE(VSExaminedPaths, (), "", "The following paths were examined for
DECLARE_MESSAGE(VSNoInstances, (), "", "Could not locate a complete Visual Studio instance")
DECLARE_MESSAGE(WaitingForChildrenToExit, (), "", "Waiting for child processes to exit...")
DECLARE_MESSAGE(WaitingToTakeFilesystemLock, (msg::path), "", "waiting to take filesystem lock on {path}...")
DECLARE_MESSAGE(WaitUntilPackagesUploaded, (msg::count), "", "Wait until the remaining packages ({count}) are uploaded")
DECLARE_MESSAGE(WarningsTreatedAsErrors, (), "", "previous warnings being interpreted as errors")
DECLARE_MESSAGE(WarnOnParseConfig, (msg::path), "", "Found the following warnings in configuration {path}:")
DECLARE_MESSAGE(WhileCheckingOutBaseline, (msg::commit_sha), "", "while checking out baseline {commit_sha}")
Expand Down
23 changes: 23 additions & 0 deletions include/vcpkg/base/message_sinks.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include <vcpkg/base/messages.h>

#include <mutex>
#include <string>
#include <vector>

Expand Down Expand Up @@ -79,4 +80,26 @@ namespace vcpkg
virtual void println(Color color, const LocalizedString& line) override;
virtual void println(Color color, LocalizedString&& line) override;
};

struct BGMessageSink final : MessageSink
{
BGMessageSink(MessageSink& out_sink) : out_sink(out_sink) { }
~BGMessageSink() { publish_directly_to_out_sink(); }
// must be called from producer
virtual void println(const MessageLine& line) override;
virtual void println(MessageLine&& line) override;
using MessageSink::println;

// must be called from consumer (synchronizer of out)
void print_published();

void publish_directly_to_out_sink();

private:
MessageSink& out_sink;

std::mutex m_published_lock;
std::vector<MessageLine> m_published;
bool m_print_directly_to_out_sink = false;
};
}
2 changes: 2 additions & 0 deletions include/vcpkg/base/strings.h
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ namespace vcpkg::Strings

const char* find_first_of(StringView searched, StringView candidates);

[[nodiscard]] std::string::size_type find_last(StringView searched, char c);

[[nodiscard]] std::vector<StringView> find_all_enclosed(StringView input,
StringView left_delim,
StringView right_delim);
Expand Down
51 changes: 44 additions & 7 deletions include/vcpkg/binarycaching.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
#include <vcpkg/fwd/tools.h>
#include <vcpkg/fwd/vcpkgpaths.h>

#include <vcpkg/base/batch-queue.h>
#include <vcpkg/base/downloads.h>
#include <vcpkg/base/expected.h>
#include <vcpkg/base/message_sinks.h>
#include <vcpkg/base/path.h>

#include <vcpkg/archives.h>
Expand All @@ -20,6 +22,7 @@
#include <iterator>
#include <set>
#include <string>
#include <thread>
#include <unordered_map>
#include <vector>

Expand Down Expand Up @@ -200,29 +203,63 @@ namespace vcpkg
std::vector<CacheAvailability> precheck(View<InstallPlanAction> actions);

protected:
BinaryProviders m_config;
struct ReadOnlyBinaryCacheData
{
BinaryProviders m_config;

std::unordered_map<std::string, CacheStatus> m_status;
std::unordered_map<std::string, CacheStatus> m_status;
};
std::unique_ptr<ReadOnlyBinaryCacheData> data = std::make_unique<ReadOnlyBinaryCacheData>();
};

// compression and upload of binary cache entries happens on a single 'background' thread, `m_push_thread`
// Thread safety is achieved within the binary cache providers by:
// 1. Only using one thread in the background for this work.
// 2. Forming a queue of work for that thread to consume in `m_actions_to_push`, which maintains its own thread
// safety
// 3. Sending any replies from the background thread through `m_bg_msg_sink`
// 4. Ensuring any supporting data, such as tool exes, is provided before the background thread is started.
// 5. Ensuring that work is not submitted to the background thread until the corresponding `packages` directory to
// upload is no longer being actively touched by the foreground thread.
struct BinaryCache : ReadOnlyBinaryCache
BillyONeal marked this conversation as resolved.
Show resolved Hide resolved
{
static ExpectedL<BinaryCache> make(const VcpkgCmdArguments& args, const VcpkgPaths& paths, MessageSink& sink);

BinaryCache(const Filesystem& fs);
BinaryCache(const BinaryCache&) = delete;
BinaryCache(BinaryCache&&) = default;
~BinaryCache();

/// Called upon a successful build of `action` to store those contents in the binary cache.
void push_success(CleanPackages clean_packages, const InstallPlanAction& action);

void print_push_success_messages();
void wait_for_async_complete_and_join();

private:
BinaryCache(BinaryProviders&& providers, const Filesystem& fs);

const Filesystem& m_fs;
Optional<ZipTool> m_zip_tool;
bool m_needs_nuspec_data = false;
bool m_needs_zip_file = false;
struct ActionToPush
{
BinaryPackageWriteInfo request;
CleanPackages clean_after_push;
};
struct BinaryCacheData
{
BinaryCacheData(const Filesystem& fs, ReadOnlyBinaryCacheData* data);
const Filesystem& m_fs;
Optional<ZipTool> m_zip_tool;
bool m_needs_nuspec_data = false;
bool m_needs_zip_file = false;

BGMessageSink m_bg_msg_sink;
BGThreadBatchQueue<ActionToPush> m_actions_to_push;
std::atomic_int m_remaining_packages_to_push = 0;
std::thread m_push_thread;

void push_thread_main(ReadOnlyBinaryCacheData* ro_data);
void wait_for_async_complete_and_join();
};
std::unique_ptr<BinaryCacheData> bc_data;
};

ExpectedL<AssetCachingSettings> parse_download_configuration(const Optional<std::string>& arg);
Expand Down
2 changes: 2 additions & 0 deletions locales/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1720,6 +1720,8 @@
"_VersionSpecMismatch.comment": "An example of {path} is /foo/bar. An example of {expected_version} is 1.3.8. An example of {actual_version} is 1.3.8.",
"VersionVerifiedOK": "{version_spec} is correctly in the version database ({git_tree_sha})",
"_VersionVerifiedOK.comment": "An example of {version_spec} is zlib:[email protected]. An example of {git_tree_sha} is 7cfad47ae9f68b183983090afd6337cd60fd4949.",
"WaitUntilPackagesUploaded": "Wait until the remaining packages ({count}) are uploaded",
"_WaitUntilPackagesUploaded.comment": "An example of {count} is 42.",
"WaitingForChildrenToExit": "Waiting for child processes to exit...",
"WaitingToTakeFilesystemLock": "waiting to take filesystem lock on {path}...",
"_WaitingToTakeFilesystemLock.comment": "An example of {path} is /foo/bar.",
Expand Down
8 changes: 8 additions & 0 deletions src/vcpkg-test/strings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ TEST_CASE ("find_first_of", "[strings]")
REQUIRE(find_first_of("abcdefg", "gb") == std::string("bcdefg"));
}

TEST_CASE ("find_last", "[strings]")
{
using vcpkg::Strings::find_last;
REQUIRE(find_last("abcdefg", 'a') == 0);
REQUIRE(find_last("abcdefg", 'g') == 6);
REQUIRE(find_last("abcdefg", 'z') == std::string::npos);
}

TEST_CASE ("contains_any_ignoring_c_comments", "[strings]")
{
using Strings::contains_any_ignoring_c_comments;
Expand Down
62 changes: 62 additions & 0 deletions src/vcpkg/base/message_sinks.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include <vcpkg/base/file_sink.h>
#include <vcpkg/base/message_sinks.h>
#include <vcpkg/base/strings.h>

namespace
{
Expand Down Expand Up @@ -279,4 +280,65 @@ namespace vcpkg
m_first.println(color, line);
m_second.println(color, std::move(line));
}

void BGMessageSink::println(const MessageLine& line)
{
std::lock_guard<std::mutex> lk(m_published_lock);
if (m_print_directly_to_out_sink)
{
out_sink.println(line);
return;
}

m_published.push_back(line);
}

void BGMessageSink::println(MessageLine&& line)
{
std::lock_guard<std::mutex> lk(m_published_lock);
if (m_print_directly_to_out_sink)
{
out_sink.println(std::move(line));
return;
}

m_published.push_back(std::move(line));
}

void BGMessageSink::print_published()
{
std::vector<MessageLine> tmp;
for (;;)
{
{
std::lock_guard<std::mutex> lk(m_published_lock);
swap(tmp, m_published);
}

if (tmp.empty())
{
return;
}

for (auto&& line : tmp)
{
out_sink.println(std::move(line));
}

tmp.clear();
}
}

void BGMessageSink::publish_directly_to_out_sink()
{
std::lock_guard<std::mutex> lk(m_published_lock);

m_print_directly_to_out_sink = true;
for (auto&& msg : m_published)
{
out_sink.println(std::move(msg));
}

m_published.clear();
}
}
6 changes: 6 additions & 0 deletions src/vcpkg/base/strings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,12 @@ const char* Strings::find_first_of(StringView input, StringView chars)
return std::find_first_of(input.begin(), input.end(), chars.begin(), chars.end());
}

std::string::size_type Strings::find_last(StringView searched, char c)
{
auto iter = std::find(searched.rbegin(), searched.rend(), c);
return iter == searched.rend() ? std::string::npos : (&*iter - searched.begin());
}

std::vector<StringView> Strings::find_all_enclosed(StringView input, StringView left_delim, StringView right_delim)
{
auto it_left = input.begin();
Expand Down
Loading