Repository navigation
Add optional FlatBuffers RPC support - #3559
Conversation
0934686 to
48177aa
Compare
|
Hi @chenBright @wwbmmm @zchuango @darion-yaphet, thanks again for all the reviews on #3558! #3559 is the last PR in the FlatBuffers series: the optional Could you please take a look when you have time? Any feedback is welcome. Thanks! |
48177aa to
8cbb17b
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
_options.protocolis configured as a protocol name (e.g. \"fb_rpc\" / \"baidu_std\"), but here… · New_options.protocolis configured as a protocol name (e.g. \"fb_rpc\" / \"baidu_std\"), but here… · Newstd::unordered_setis used in this file, but the FlatBuffers include block only adds<map>in… · New
What changed in this PR
This PR adds optional FlatBuffers RPC (“fb_rpc”) transport support on top of the existing FlatBuffers message/service generation work, plus an end-to-end example and a dedicated FlatBuffers-ON validation workflow.
Changes:
- Integrates
fb_rpcas a new built-inProtocolwith Channel/Controller/Server support, service registration, and concurrency integration. - Adds comprehensive FlatBuffers protocol/unit tests and wires them into CMake/Make/Bazel test builds.
- Adds a generated client/server example with a bounded smoke test, updated documentation (EN/CN), and a GitHub Actions “FlatBuffers ON” gate runner.
| File | Description |
|---|---|
| tools/flatbuffers/README.md | Updates generator README to point to runtime enablement + fb_rpc usage and verification guidance. |
| test/brpc_flatbuffers_protocol_unittest.cpp | Adds dedicated protocol-level and lifecycle tests for fb_rpc. |
| test/brpc_channel_unittest.cpp | Avoids colliding with newly assigned built-in protocol IDs in channel tests. |
| test/Makefile | Ensures FlatBuffers protocol tests are excluded when FlatBuffers is OFF and builds/link rules include the new test. |
| test/CMakeLists.txt | Adds brpc_flatbuffers_protocol_unittest to CMake test targets when FlatBuffers is ON. |
| test/BUILD.bazel | Adds Bazel cc_test for brpc_flatbuffers_protocol_unittest and excludes it from generic globbed tests. |
| src/brpc/server.h | Introduces FlatBuffers service registration API and internal method property plumbing (guarded by BRPC_WITH_FLATBUFFERS). |
| src/brpc/server.cpp | Implements FlatBuffers service registry, versioning/stat exposure, concurrency limiter hookup, and conflict checks with protobuf services. |
| src/brpc/policy/flatbuffers_protocol.h | Declares the fb_rpc protocol hook points (parse/verify/serialize/pack/process). |
| src/brpc/policy/flatbuffers_protocol.cpp | Implements FRPC framing, request/response processing, and async completion lifecycle for FlatBuffers RPC. |
| src/brpc/options.proto | Reserves built-in PROTOCOL_FLATBUFFERS_RPC = 30. |
| src/brpc/global.cpp | Registers fb_rpc as a built-in protocol when FlatBuffers is enabled. |
| src/brpc/details/server_private_accessor.h | Exposes FlatBuffers method lookup to protocol code via server private accessor. |
| src/brpc/details/controller_private_accessor.h | Adds private setter for FlatBuffers method descriptor on Controller. |
| src/brpc/controller.h | Adds Controller::flatbuffers_method() accessor and stores FlatBuffers method descriptor pointer. |
| src/brpc/controller.cpp | Clears FlatBuffers method context in Controller::ResetPods(). |
| src/brpc/channel.h | Extends Channel to implement flatbuffers::RpcChannel and adds FBCallMethod. |
| src/brpc/channel.cpp | Routes FlatBuffers calls via FBCallMethod, adds entry validation, and adjusts tracing method-name selection. |
| src/brpc/CMakeLists.txt | Ensures flatbuffers_protocol.cpp is excluded when FlatBuffers is OFF. |
| example/benchmark_fb/smoke.py | Adds bounded end-to-end smoke test orchestration for generated FB client/server. |
| example/benchmark_fb/server.cpp | Adds example FlatBuffers server (protocol fb_rpc, service registration, graceful shutdown). |
| example/benchmark_fb/echo.fbs | Defines example schema and stable wire ID for the RPC method. |
| example/benchmark_fb/client.cpp | Adds example FlatBuffers client driving generated stub + verifies response schema/attachments. |
| example/benchmark_fb/README.md | Documents building/running the example and what the smoke test validates. |
| example/benchmark_fb/CMakeLists.txt | Adds standalone CMake build for the example + codegen steps and smoke CTest. |
| docs/en/flatbuffers.md | Expands docs to cover fb_rpc enablement, full verification steps, and limitations. |
| docs/cn/flatbuffers.md | Mirrors EN documentation updates for fb_rpc enablement and verification steps. |
| README_cn.md | Links to the CN FlatBuffers RPC guide. |
| README.md | Links to the EN FlatBuffers RPC guide. |
| Makefile | Excludes flatbuffers_protocol.cpp from Make builds when FlatBuffers is OFF. |
| CMakeLists.txt | Updates WITH_FLATBUFFERS option description to include RPC support. |
| BUILD.bazel | Includes flatbuffers_protocol.cpp in FlatBuffers-enabled Bazel source patterns. |
| .gitignore | Stops ignoring .github/ to keep workflows/scripts discoverable. |
| .github/workflows/flatbuffers-on.yml | Adds a CI workflow that runs the FlatBuffers-ON gates across CMake/Make/Bazel on Linux/macOS. |
| .github/scripts/test_flatbuffers_on.py | Adds self-tests validating the gate script’s report parsing and safety checks. |
| .github/scripts/flatbuffers-on.py | Adds an “ON runner” that builds deps, runs unit/codegen/example tests, and rejects empty/skipped runs. |
Review effort: Lite
| return; | ||
| } | ||
| cntl->set_used_by_rpc(); | ||
| const bool is_fb_protocol = (_options.protocol == PROTOCOL_FLATBUFFERS_RPC); |
| const int64_t start_send_us = butil::cpuwide_time_us(); | ||
| std::string method_name; | ||
| if (_get_method_name) { | ||
| if (_get_method_name && is_fb == is_fb_protocol) { |
| // Keep ownership with the caller until the entire record is published. | ||
| auto record = std::make_unique<FlatBuffersServiceMap::Service>(service, sd); | ||
| std::unordered_set<std::string> method_names; |
| Method* FindMethodByFullName(const butil::StringPiece& fullname) const { | ||
| for (const auto& service : services) { | ||
| for (const auto& method : service.second->methods) { | ||
| if (method.second->property.method->full_name() == fullname) { | ||
| return method.second.get(); | ||
| } | ||
| } | ||
| } | ||
| return nullptr; | ||
| } |
|
[replied by brpc-oncall robot] Thanks for splitting the RPC increment out of #3197 and for the very thorough documentation. Because this PR adds a new built-in protocol together with public API on
Note: Copilot's three findings above look like false positives to me. Thanks! |
|
Thanks for the detailed review, @wwbmmm, and for checking the Copilot comments. Below are the benchmark results for point 1 and proposals for points 2–4. 1. Motivation and benchmark
To measure it I extended
Summary (fb_rpc vs baidu_std; positive QPS is better, negative CPU is better):
Full results are in the collapsed section at the end. Builder sizing. Not covered: RDMA, attachments, the full matrix with pooled connections, and newer Protobuf releases. Missing features. Authentication, compression, checksums and streaming are out of scope for this PR. Because the metadata carries an explicit length, fields such as log-id and user fields can be appended later without breaking the known prefix. 2. ABIAgreed that the layout should not depend on
With this, a release has one layout whatever the option is, and protobuf-only users are not affected. A consumer built with ON against a library built with OFF fails at link time instead of at run time. A separate component would still need protocol registration and dispatch hooks in 3. Service registryProposal:
A shared registry abstraction for both kinds of service would touch the protobuf paths much more, so I'd prefer to do it in a follow-up PR. 4. Wire formatAgreed. Compatibility with the earlier experimental FRPC formats is not a goal, so the header and the fixed metadata will both be big-endian (network order), using the butil byte-order helpers, with a test that fixes the byte layout of request and response frames. The FlatBuffers payload stays little-endian as FlatBuffers defines it; it is not part of the frame. I'll keep the The benchmark changes to Full results (1 / 16 / 64 callers, 10 payload sizes, medians of 5 rounds)1 caller
16 callers
64 callers
For each case the two protocols ran back to back in the same round on the same server process. 15 of the 30 cases were measured in a second session (0 B, 256 B, 4 KB and 16 KB at all caller counts, and every size at 16 callers); results from the two sessions differ by a few percent on their own, so compare cases within a table row rather than across sessions. Reproduce with the example (server and client hosts separate): benchmark_fb_server --listen_addr=SERVER_IP:8901 --allow_remote --duration_s=3600 --max_concurrency=4096
for p in fb_rpc baidu_std; do
benchmark_fb_client --server=SERVER_IP:8901 --allow_remote --protocol=$p --use_bthread=true \
--duration_s=30 --warmup_s=3 --thread_num=64 --request_size=1024
done |
|
@wwbmmm Gentle ping. Could you take a look at the proposals for points 2 and 3 above? Once the direction is agreed, I'll implement them in this PR. |
I think that's ok. Please go on. |
|
LGTM |
54fc318 to
e872eae
Compare
3be3c08 to
8fd8b9d
Compare
|
FlatBuffers ON now runs in CI. The remaining failure, FB ON (ubuntu-22.04, cmake, clang), comes from master: in C++14 builds, cacheline-aligned pools are allocated with plain |
8fd8b9d to
a26ffd9
Compare
8b56540 to
fcb4267
Compare
|
@chenBright Following your suggestion on #3698, FlatBuffers now requires C++17: CMake and |
fcb4267 to
67aff8e
Compare
chenBright
left a comment
chenBright
left a comment
There was a problem hiding this comment.
src/brpc/policy/flatbuffers_protocol.cpp:51
meta will need more optional fields, strings, or nested information over time. Would it be more extensible and maintainable to define RequestMeta / ResponseMeta as FlatBuffers tables, retain the outer length and framing structure, and let the schema and generated code handle field access and encoding/decoding?
Add the optional fb_rpc transport. A frame is a 12-byte FRPC header and fixed request or response metadata, all in network byte order, followed by the FlatBuffers message and an optional attachment. Methods are addressed by stable IDs. Integrate it with Channel, Controller and Server without changing the existing protobuf Protocol callback signatures: - Channel, Controller and Server keep the same layout whether BRPC_WITH_FLATBUFFERS is on or off. Generated stubs reach a brpc::Channel through a library adapter, so `Echo::Stub stub(&channel);` works unchanged. A layout probe built in both configurations checks it. - FlatBuffers methods use Server::MethodProperty, so bvar exposure, concurrency limits, MaxConcurrencyOf() and IgnoreEovercrowdedOf() share one code path. They stay out of the map other protocols search by name. ListFlatBuffersServices() lists them and /status shows them. - A response is accepted only on the socket used by the attempt it answers. - New server connections try fb_rpc right after baidu_std. Protocol IDs and handler indices stay as they are; InputMessenger keeps a separate probe order instead. Keep request buffers immutable across retries and concurrent calls, and manage service ownership, asynchronous completion and concurrency limits. Reject invalid frames and unsupported authentication, compression, checksums and streams. FlatBuffers support requires C++17. CMake and config_brpc.sh switch to C++17 when it is enabled, and the FlatBuffers headers reject older standards, so over-aligned objects use aligned new in the library and in every unit that includes them. Protobuf-only builds are unchanged. The feature stays disabled by default in CMake, Make and Bazel. Unit tests that registered a private protocol as ID 30, now used by fb_rpc, move to ID 127. The codegen test lists the selected Protobuf include directories before shared prefixes. Add 43 protocol tests covering the wire format, service lifecycle, registry, malformed input and real RPC round trips.
Add a client/server example built from a generated service. By default it runs a bounded loopback smoke test: 13 verified replies, 2 expected schema rejections and a clean shutdown. An opt-in timed mode compares fb_rpc with an equivalent protobuf service over baidu_std on the same port. --duration_s and --warmup_s run closed-loop callers and print QPS, latency percentiles and client CPU as one JSON line; --protocol, --use_bthread and --allow_remote select the protocol, bthread callers and remote endpoints. The measured window and its CPU sample start and end exactly where counting starts and stops, so calls still in flight when the timer expires do not stretch it. The smoke test checks this against a server started with --reply_delay_ms. Request and reply builders reserve the payload size, so large messages are not grown by repeated reallocate-and-copy. The example takes an installed FlatBuffers-enabled bRPC, writes the code generated by flatc and brpc_flatc into its own build directory, and lists the selected Protobuf include directories before shared prefixes.
Add a runner that builds and tests the FlatBuffers-enabled configuration with CMake, Make or Bazel, rejects empty, filtered or skipped test runs, and compares the class layout probes built with FlatBuffers on and off. Add its self-tests and a GitHub Actions workflow that runs it on Linux and macOS when FlatBuffers-related paths change. The jobs build gperftools as ci-linux.yml does. On macOS they use protobuf@29 followed by the shared Homebrew prefix, as ci-macos.yml does.
Add Chinese and English guides covering how to enable FlatBuffers, the dependencies and the C++17 requirement, building the runtime, running the example with its smoke test and timed comparison, the network RPC API, builder sizing for large messages and the FRPC wire format. Update the README files.
67aff8e to
a7f9ce6
Compare
|
@chenBright Thanks, agreed.
Small-packet A/B against the previous head (fb_rpc, cross-host TCP, single |
RunTimed ignored worker start failures and could exit 0 with fewer callers than requested. Exit 1 in that case, report started_threads, and cover it in smoke.py with a test-only --fail_worker_start_at flag.
- Keep timed-mode latency in fixed-size per-caller histograms instead of one sample per call, so memory no longer grows with duration_s. - Time only the RPC, and append the attachment IOBuf by reference and compare it without copying. - Reject timed runs over short connections, which exhaust local ports. - Close the window early and exit 1 on SIGINT/SIGTERM. - Do not verify the request schema twice on the fb_rpc server. - Accept EINTERNAL as well as -1 for schema rejection. - Document server lifetime, max_concurrency and payload_mib_per_s.
There is no need for |
FlatBuffers support requires C++17, so the codegen CI does not need to cover older standards. The headers still #error below C++17.
|
@chenBright Thanks. Removed the C++14 rejection check from the codegen acceptance test in ba26507; the FlatBuffers headers still |
|
@wwbmmm PTAL. |
What problem does this PR solve?
This continues the RPC work in #3197 on top of #3558, which is now merged. #3558 added the FlatBuffers messages, builders, service descriptors and the
brpc_flatcgenerator; this PR adds the optionalfb_rpctransport on top of it.The branch is rebased onto master after #3558 and contains only the four RPC commits.
What is changed and the side effects?
Add FlatBuffers RPC protocol support: optionalfb_rpctransport, Channel/Controller/Server integration, service lifecycle handling, attachments, retries and concurrency, with 33 protocol tests.Add a generated FlatBuffers RPC example: generated client/server example with a bounded loopback smoke test. This is functional validation, not a performance benchmark.Add explicit FlatBuffers ON validation: an ON validation runner, its self-tests and a GitHub Actions workflow.Document FlatBuffers setup and example checks: Chinese and English setup, dependency, ABI and example-verification documentation.The transport uses explicit metadata lengths, so optional fields can be appended later while the known prefix stays readable. Existing protobuf-facing
Protocolcallback signatures are unchanged.Side effects and limits:
output/include. The ON runner and the docs now install the runtime into a prefix before building the example.Validation
Run on the rebased head
48177aa2:LINK_SO=ON)python3 .github/scripts/test_flatbuffers_on.pyThe example smoke verifies 13 replies, 2 expected schema rejections and a clean shutdown.
Not covered by these runs: Make and Bazel ON modes (left to CI), sanitizer runs and performance benchmarks.
Check List: