Feat: Introduce Mbox C API for multiple parties communication #567

tongke6 · 2025-09-11T08:25:11Z

No description provided.

gemini-code-assist

Summary of Changes

Hello @tongke6, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request establishes a foundational interface for inter-party communication within the yacl::link module. By introducing the Mbox interface and an in-memory implementation, it provides a flexible and testable mechanism for exchanging messages between different participants. This enhancement is crucial for building and verifying distributed protocols, offering a clear abstraction layer for communication channels.

Highlights

New Mbox Interface: Introduced a new virtual interface, yacl::link::Mbox, for abstracting multi-party communication, defining Send and Recv methods.
In-Memory Mbox Implementation: Provided an InMemoryMbox class, which is a concrete implementation of the Mbox interface, designed for testing and local development scenarios.
Mbox Factory: Added a factory function, CreateInMemoryMboxs, to easily create multiple interconnected InMemoryMbox instances for a given world_size.
Unit Tests: Included comprehensive unit tests for InMemoryMbox covering basic send/receive, multiple messages, timeout handling, and concurrent operations to ensure correctness.
Bazel Build Updates: Updated the Bazel build configuration to include the new mbox and mbox_factory libraries and their corresponding tests, along with a minor update to MODULE.bazel.lock.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a new Mbox interface for multi-party communication, along with an in-memory implementation for testing purposes. The overall design is sound. My review focuses on improving performance, code hygiene, and style consistency. I've suggested using absl::StrCat for more efficient string construction, which requires a small build file modification. I've also pointed out some unused header includes and recommended adding missing newlines at the end of the new files for better style compliance.

gemini-code-assist · 2025-09-11T08:27:53Z

yacl/link/BUILD.bazel

+    deps = [
+        ":mbox",
+    ],


To support the suggested use of absl::StrCat for more efficient string building in mbox_factory.cc, the absl/strings dependency should be added here.

deps = [ ":mbox", "@abseil-cpp//absl/strings", ],

gemini-code-assist · 2025-09-11T08:27:53Z

yacl/link/mbox.h

+                                                   int64_t timeout_ms) = 0;
+};
+
+}  // namespace yacl::link


It is a common convention and good practice to end files with a newline character. Some tools and compilers may issue a warning if it's missing.

Suggested change

} // namespace yacl::link

} // namespace yacl::link

gemini-code-assist · 2025-09-11T08:27:53Z

yacl/link/mbox_factory.cc

+
+#include <chrono>
+#include <exception>
+#include <future>


This header is included but not used in this file. Removing it can improve compilation speed and reduce unnecessary dependencies.

gemini-code-assist · 2025-09-11T08:27:54Z

yacl/link/mbox_factory.cc

+#include <map>
+#include <mutex>
+#include <optional>
+#include <shared_mutex>


This header is included but not used in this file. Removing it can improve compilation speed and reduce unnecessary dependencies.

gemini-code-assist · 2025-09-11T08:27:54Z

yacl/link/mbox_factory.cc

+  std::string full_key =
+      "/frm/" + std::to_string(rank_) + "/msg/" + std::string(key);


This string concatenation creates multiple temporary std::string objects, which is inefficient. Using absl::StrCat is a more performant way to concatenate strings as it avoids intermediate allocations.

To use absl::StrCat, you'll also need to:

Add #include "absl/strings/str_cat.h" to this file.

Add the @abseil-cpp//absl/strings dependency in yacl/link/BUILD.bazel (I've added a separate comment for this).

Suggested change

std::string full_key =

"/frm/" + std::to_string(rank_) + "/msg/" + std::string(key);

std::string full_key = absl::StrCat("/frm/", rank_, "/msg/", key);

gemini-code-assist · 2025-09-11T08:27:54Z

yacl/link/mbox_factory.cc

+  std::string full_key =
+      "/frm/" + std::to_string(src) + "/msg/" + std::string(key);


Similar to the Send method, this string concatenation is inefficient. It's recommended to use absl::StrCat for better performance.

Suggested change

std::string full_key =

"/frm/" + std::to_string(src) + "/msg/" + std::string(key);

std::string full_key = absl::StrCat("/frm/", src, "/msg/", key);

gemini-code-assist · 2025-09-11T08:27:54Z

yacl/link/mbox_factory.cc

+  return result;
+}
+
+}  // namespace yacl::link


It is a common convention and good practice to end files with a newline character. Some tools and compilers may issue a warning if it's missing.

Suggested change

} // namespace yacl::link

} // namespace yacl::link

gemini-code-assist · 2025-09-11T08:27:54Z

yacl/link/mbox_factory.h

+// from 0 to world_size - 1.
+std::vector<std::shared_ptr<Mbox>> CreateInMemoryMboxs(size_t world_size);
+
+}  // namespace yacl::link


It is a common convention and good practice to end files with a newline character. Some tools and compilers may issue a warning if it's missing.

Suggested change

} // namespace yacl::link

} // namespace yacl::link

gemini-code-assist · 2025-09-11T08:27:54Z

yacl/link/mbox_factory_test.cc

+  receiver.join();
+}
+
+}  // namespace yacl::link


It is a common convention and good practice to end files with a newline character. Some tools and compilers may issue a warning if it's missing.

Suggested change

} // namespace yacl::link

} // namespace yacl::link

FollyCoolly · 2025-09-11T09:31:08Z

yacl/link/mbox_factory.cc

+
+  std::optional<std::vector<uint8_t>> result = std::nullopt;
+
+  auto stop_wait = [&] {


可以使用 extract：

auto stop_wait = [&] {
auto& mailbox = shared_state_->mailboxes[rank_];
if (auto node = mailbox.extract(full_key); !node.empty()) {
result = std::move(node.mapped());
return true;
}
return false;
};

FollyCoolly · 2025-09-11T09:40:30Z

yacl/link/mbox_factory_test.cc

+  EXPECT_FALSE(received.has_value());
+}
+
+TEST(InMemoryMbox, ConcurrentSendRecv) {


看起来Concurrent不是重点，BlockingRecvThenSend 如何？

Jamie-Cui · 2025-09-12T06:32:59Z

yacl/link/mbox_factory.h

+  std::optional<std::vector<uint8_t>> Recv(size_t src, std::string_view key,
+                                           int64_t timeout_ms) override;
+
+ public:


multiple "public"s in a class

yacl/link/mbox_wrapper.h

rivertalk · 2025-09-15T07:28:34Z

yacl/link/mbox_capi.h

+/// @param data_len Length of data in bytes.
+///
+/// @return MBOX_SUCCESS on success, appropriate error code on failure.
+mbox_error_t mbox_send(mbox_t* mbox, size_t dst, const char* key,


这个API有个隐藏语义

send到底是放到自己的mbox 就success了，还是到达对方的 mbox 才算success

理论上我理解放到自己的mbox就算成功了，这样对异步比较友好，但也有两个问题

sender 如何知道 mbox 之间的发送错误，需要新的接口吗？

mobx 是否本身也要 flush 接口，Sender 异步send完毕后结束进程的问题

目前想的是放到自己的发送队列里就算成功。

flush 接口可以按需加。
mbox 后续会增加stats 接口，统计发送成功的，正在发送的，以及发送失败的等统计信息，以及接受信息。

github-actions · 2025-10-15T09:44:59Z

Stale pull request message. Please comment to remove stale tag. Otherwise this pr will be closed soon.

Feat: Introduce Mbox interface for multiple parties communication

45e0bf9

tongke6 requested review from danqi437 and rivertalk September 11, 2025 08:25

gemini-code-assist bot reviewed Sep 11, 2025

View reviewed changes

tongke6 requested a review from FollyCoolly September 11, 2025 08:25

gemini-code-assist bot reviewed Sep 11, 2025

View reviewed changes

FollyCoolly reviewed Sep 11, 2025

View reviewed changes

Jamie-Cui reviewed Sep 12, 2025

View reviewed changes

tongke6 force-pushed the tk/mailbox branch from 44d173f to 847da1c Compare September 12, 2025 06:35

tongke6 marked this pull request as draft September 12, 2025 06:38

tongke6 force-pushed the tk/mailbox branch from 847da1c to a19eb5b Compare September 15, 2025 03:44

tongke6 changed the title ~~Feat: Introduce Mbox interface for multiple parties communication~~ Feat: Introduce Mbox C API for multiple parties communication Sep 15, 2025

tongke6 force-pushed the tk/mailbox branch 2 times, most recently from 5baf9df to 36ca71d Compare September 15, 2025 07:00

tongke6 marked this pull request as ready for review September 15, 2025 07:01

Feat: add mbox C-API for multiple parties communication

4df2e02

tongke6 force-pushed the tk/mailbox branch from 36ca71d to 4df2e02 Compare September 15, 2025 07:06

rivertalk reviewed Sep 15, 2025

View reviewed changes

github-actions bot added the no-pr-activity label Oct 15, 2025

tongke6 removed the no-pr-activity label Oct 15, 2025

Niko-Zeng added this to 隐语SecretFlow激励活动 Oct 16, 2025

Niko-Zeng removed this from 隐语SecretFlow激励活动 Oct 16, 2025

		std::string full_key =
		"/frm/" + std::to_string(rank_) + "/msg/" + std::string(key);

	std::string full_key =
	"/frm/" + std::to_string(rank_) + "/msg/" + std::string(key);
	std::string full_key = absl::StrCat("/frm/", rank_, "/msg/", key);

		std::string full_key =
		"/frm/" + std::to_string(src) + "/msg/" + std::string(key);


		std::optional<std::vector<uint8_t>> result = std::nullopt;

		auto stop_wait = [&] {

Feat: Introduce Mbox C API for multiple parties communication #567

Are you sure you want to change the base?

Feat: Introduce Mbox C API for multiple parties communication #567

Uh oh!

Conversation

tongke6 commented Sep 11, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

FollyCoolly Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

FollyCoolly Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Jamie-Cui Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rivertalk Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

tongke6 Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants