Feature #64375
closedcrimson: introduce support for C++ coroutines
0%
Description
TLDR: we'll need to use gcc13 or clang 16/17
Chaining lambdas as continuations to futures is relatively error prone as the developer must reason carefully about captured reference lifetimes. C++ coroutines should help with this as a developer can co_await a future without losing the currently scoped variables.
Seastar already has support for using seastar::future's with C++ coroutines. The PRs below add support for crimson's future wrappers -- including static checking of errorated futures and support for checking interruptions upon resume for interruptible futures.
I hit three serious code generation bugs with gcc 11.4.1. One seems to be fixed in 12.2.1, the other two seem to be fixed in 13.2.1. Testing on clang 17 didn't show any problems, and I hear from Kefu that clang 16 should be ok as well. Thus, the plan is:
- https://github.com/ceph/ceph/pull/55846 - initial coroutine support for errorators and interruptible futures
- https://github.com/ceph/ceph/pull/55847 - converts some client_request code -- see perf results below
- https://github.com/ceph/ceph/pull/55825 - converts crimson builds to use clang by default
- update make check hosts to use at least clang 16
- update CMakeLists to require gcc>=13 or clang>=16
GCC Bugs:
- https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401
The specific symptom I observed is the pg param being
destructed multiple times resulting in the refcount going
rapidly to 0 destroying the PG prematurely.
- https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102217
This one appears to cause the generated code to double-free
the awaiter holding the future. This one seems to be fixed
in gcc 13.2.1.
- https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101244
This example isn't precisely as described in the bug, but it seems
similar. It causes the generated code to incorrectly execute
process_pg_op unconditionally before the predicate. It seems to be
fixed in gcc 12.2.1.
Perf result summary:
With writes, the non-coroutine version seems to have a small edge, but
the effect is reversed with reads. For the moment, I'm satisfied that
it's at least performant enough for most of the code base.
gcc version 13.2.1 20231205 (Red Hat 13.2.1-6) (GCC)
both branches have the following applied to src/seastar/CMakeLists.txt to allow
building with gcc 13:
-if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") - if (NOT Cxx_Compiler_BZ107852_Free AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 13) - include (CheckGcc107852) - list (APPEND Seastar_PRIVATE_CXX_FLAGS - -Wno-error=stringop-overflow - -Wno-error=array-bounds) - endif() -endif () -
sjust/wip-crimson-coroutines 4c5681c099752b5e36e3a67859b59362343c1ed0
Block_size 4096 4096 4096
Time 100 100 100
Core 2 4 8
Tool Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029
OPtype Rand Write Rand Write Rand Write
CommitID 4c5681c0 4c5681c0 4c5681c0
OSD Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore
Thread_num 128 128 128
Client_num 4 4 4
Latency(ms) 56.6480625 23.11821 9.99745
Bandwidth(MB/s) 37.0182656 90.7411251199 157.70769408
IOPS 9037.66 22153.61 38502.87sjust/wip-crimson-coroutines-base e160811c5fec46717a117ac02b6b29609f067233
Block_size 4096 4096 4096
Time 100 100 100
Core 2 4 8
Tool Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029
OPtype Rand Write Rand Write Rand Write
CommitID e160811c e160811c e160811c
OSD Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore
Thread_num 128 128 128
Client_num 4 4 4
Latency(ms) 55.26808 22.9293124999 12.9919900000
Bandwidth(MB/s) 37.94628608 91.4753433600 161.4054912
IOPS 9264.22 22332.86 39405.64gcc version 11.4.1 20230605 (Red Hat 11.4.1-2) (GCC)
No changes, CMakeLists change above not applied.
sjust/wip-crimson-coroutines 4c5681c099752b5e36e3a67859b59362343c1ed0
Block_size 4096 4096 4096
Time 100 100 100
Core 2 4 8
Tool Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029
OPtype Rand Write Rand Write Rand Write
CommitID 4c5681c0 4c5681c0 4c5681c0
OSD Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore
Thread_num 128 128 128
Client_num 4 4 4
Latency(ms) 57.35153 23.1843599999 9.9319775
Bandwidth(MB/s) 36.57537536 90.4843059200 158.11516416
IOPS 8929.54 22090.89 38602.35Block_size 4096 4096 4096
Time 100 100 100
Core 2 4 8
Tool Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029
OPtype Rand Write Rand Write Rand Write
CommitID 4c5681c0 4c5681c0 4c5681c0
OSD Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore
Thread_num 128 128 128
Client_num 4 4 4
Latency(ms) 57.1692875 23.2074850000 13.169925
Bandwidth(MB/s) 36.6791475199 90.39757312 159.24107264
IOPS 8954.87 22069.7 38877.21Block_size 4096 4096 4096
Time 100 100 100
Core 2 4 8
Tool Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029
OPtype Rand Write Rand Write Rand Write
CommitID 4c5681c0 4c5681c0 4c5681c0
OSD Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore
Thread_num 128 128 128
Client_num 4 4 4
Latency(ms) 56.8740425 23.188265 13.3796974999
Bandwidth(MB/s) 36.87575552 90.49010176 156.84764672
IOPS 9002.89 22092.32 38292.88sjust/wip-crimson-coroutines-base e160811c5fec46717a117ac02b6b29609f067233
Block_size 4096 4096 4096
Time 100 100 100
Core 2 4 8
Tool Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029
OPtype Rand Write Rand Write Rand Write
CommitID e160811c e160811c e160811c
OSD Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore
Thread_num 128 128 128
Client_num 4 4 4
Latency(ms) 55.7295474999 22.8178975000 12.99293
Bandwidth(MB/s) 37.63249152 91.93051136 161.46226176
IOPS 9187.62 22443.99 39419.51Block_size 4096 4096 4096
Time 100 100 100
Core 2 4 8
Tool Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029
OPtype Rand Write Rand Write Rand Write
CommitID e160811c e160811c e160811c
OSD Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore
Thread_num 128 128 128
Client_num 4 4 4
Latency(ms) 55.38967 23.1957575000 12.9069425
Bandwidth(MB/s) 37.8617856000 90.47964672 162.6132992
IOPS 9243.6 22089.77 39700.53With reads, the coroutine version seems to have a very small edge:
sjust/wip-crimson-coroutines 4c5681c099752b5e36e3a67859b59362343c1ed0 (clang 17.0.1)
Block_size 4096 4096 4096 4096 4096 4096
Time 120 120 120 120 120 120
Core 8 8 8 8 8 8
Tool Fio RBD Fio RBD Fio RBD Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029 2024029 2024029 2024029
OPtype Rand Read Rand Read Rand Read Rand Read Rand Read Rand Read
CommitID 4c5681c0 4c5681c0 4c5681c0 4c5681c0 4c5681c0 4c5681c0
OSD Crimson Crimson Crimson Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore Bluestore Bluestore Bluestore
Thread_num 128 128 128 128 128 128
Client_num 4 4 4 4 4 4
Latency(ms) 7.948315 7.8127875 7.89334249999 7.84073999999 7.93500000000 7.94527749999
Bandwidth(MB/s) 263.88103168 268.616540159 265.790945279 267.56045824 264.432537600 263.981690880
IOPS 64424.09 65580.19 64890.37 65322.3899999 64558.7400000 64448.66sjust/wip-crimson-coroutine-base e160811c5fec46717a117ac02b6b29609f067233 (clang 17.0.1)
Block_size 4096 4096 4096 4096 4096 4096
Time 120 120 120 120 120 120
Core 8 8 8 8 8 8
Tool Fio RBD Fio RBD Fio RBD Fio RBD Fio RBD Fio RBD
Version 2024029 2024029 2024029 2024029 2024029 2024029
OPtype Rand Read Rand Read Rand Read Rand Read Rand Read Rand Read
CommitID e160811c e160811c e160811c e160811c e160811c e160811c
OSD Crimson Crimson Crimson Crimson Crimson Crimson
Store Bluestore Bluestore Bluestore Bluestore Bluestore Bluestore
Thread_num 128 128 128 128 128 128
Client_num 4 4 4 4 4 4
Latency(ms) 8.020955 8.00303749999 7.97546750000 7.95904750000 7.9118425 7.825425
Bandwidth(MB/s) 261.57848576 263.037184 263.10528 263.61894912 265.24925952 268.297041919
IOPS 63861.95 64218.0400000 64234.7100000 64360.1 64758.1499999 65502.22Updated by Kefu Chai 3 months ago
Hi Sam, the older versions of GCC and Clang lack sound support of C++20 coroutines. that's why both ScyllaDB and Redpanda are using Clang. ScyllaDB uses Clang 17, while Redpanda Clang 16.
Since the C++23 standard is around the corner, see https://www.iso.org/standard/83626.html. Seastar dropped the C++17 support yesterday, see https://github.com/scylladb/seastar/commit/5d3ee980730b49c2379a2728d69698ee68f5dbeb. so that it can use coroutine without guarding it with the C++20 checks. so if crimson plans to bump its seastar submodule every now and then, it would have to be prepared for the C++20 only changes.
regarding the performance, this reminds me a talk by Travis on the P99 conf, see https://www.youtube.com/watch?v=pOIoeFuIUKQ . but IMHO, we should not abandon C++20 coroutine just because of its limitations, as not all code paths are mission critical, especially those on the the control plane, and even those on some background data paths. so is switching from Clang instead of GCC for compiling crimson a viable solution, before GCC can catch up with Clang in this regards?
Updated by Samuel Just 3 months ago
I don't really have bandwidth right now to deal with switching to clang, and I'm not sure I want to try supporting clang in production builds. Also, I'm not abandoning coroutines, just delaying them until compiler support is more readily reliable.
The observation on performance isn't fatal. The delta is quite small (though a bit troubling relative to the total amount of code converted) and is probably fixable even in performance critical code. However, because gcc support is demonstrably not ready and it doesn't provide a performance benefit, I'm inclined to delay it for now.
Updated by Samuel Just 3 months ago
We will, of course, continue updating to match seastar trunk. If that ends up driving a switch to clang or gcc13 then I'll probably revisit the issue.
Updated by Samuel Just 3 months ago
Eh, maybe we'll end up looking into it sooner. Thanks for the info on the clang versions you're using -- might find time this week to try clang 16/17.
Updated by Samuel Just 2 months ago
It seems there are other reasons to switch compilers, I'm looking into using clang for crimson builds now.
Updated by Samuel Just 2 months ago
- Assignee set to Samuel Just
- Priority changed from Low to High
https://github.com/ceph/ceph/pull/55846 https://github.com/ceph/ceph/pull/55847 add initial support and convert some code with workarounds for gcc-11's failures. https://github.com/ceph/ceph/pull/55825 will switch crimson over to building with clang. Once that merges, we can update the cmake files to require gcc>=13 or clang>=16 and drop the work arounds.