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.

Sign up for GitHub

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

Jump to bottom

Improve compilation and linking of Rust files #11102

Closed
wants to merge 2 commits into from
Closed

Improve compilation and linking of Rust files #11102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b7b8a7
rust: adjust build according to cxxbridge's recommendations
wmitros Jun 23, 2022
f032b40
rust: add build profiles more aligned with ninja modes
wmitros Jun 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 31 additions & 17 deletions configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,14 @@ def find_headers(repodir, excluded_dirs):
'raft/log.cc',
]

rusts = [
'rust/src/lib.rs',
'rust/Cargo.toml',
'rust/Cargo.lock',
'rust/inc/src/lib.rs',
'rust/inc/Cargo.toml',
]

scylla_core = (['message/messaging_service.cc',
'replica/database.cc',
'replica/table.cc',
Expand Down Expand Up @@ -1153,10 +1161,6 @@ def find_headers(repodir, excluded_dirs):
'idl/position_in_partition.idl.hh',
]

rusts = [
'rust/inc/src/lib.rs',
]

headers = find_headers('.', excluded_dirs=['idl', 'build', 'seastar', '.git'])

scylla_tests_generic_dependencies = [
Expand Down Expand Up @@ -1309,7 +1313,7 @@ def find_headers(repodir, excluded_dirs):

deps['test/boost/duration_test'] += ['test/lib/exception_utils.cc']
deps['test/boost/schema_loader_test'] += ['tools/schema_loader.cc']
deps['test/boost/rust_test'] += rusts
deps['test/boost/rust_test'] += ['rust/inc/src/lib.rs']

deps['test/raft/replication_test'] = ['test/raft/replication_test.cc', 'test/raft/replication.cc', 'test/raft/helpers.cc'] + scylla_raft_dependencies
deps['test/raft/randomized_nemesis_test'] = ['test/raft/randomized_nemesis_test.cc', 'direct_failure_detector/failure_detector.cc', 'test/raft/helpers.cc'] + scylla_raft_dependencies
Expand Down Expand Up @@ -1411,7 +1415,7 @@ def clang_inline_threshold():
for mode in modes:
modes[mode]['cxxflags'] += f' -Wstack-usage={modes[mode]["stack-usage-threshold"]} -Wno-error=stack-usage='

has_wasmtime = os.path.isfile('/usr/lib64/libwasmtime.a') and os.path.isdir('/usr/local/include/wasmtime')
has_wasmtime = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this because linking with both libwasmtime and librust_combined was causing duplicate symbol errors. This will not be an issue anymore when we add wasmtime in rust, and that patch is only waiting for #10306, so this temporary fix should not last a long time (wasm is also still experimental)

Copy link
Contributor

Choose a reason for hiding this comment

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

But this "fix" means that wasm will no longer be working at all, until a later fix? @psarna are we fine with that?
I see 10306 has already been merged, so can't you fix that wasm issue right now and not have any temporary period where it doesn't work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I missed that one. No, it's not acceptable, we have experimental support, but we also already published a few examples of it online and we expect people to try it out, not to find out they're missing in a particular commit. What should be done here is not linking with librust_combined now if it's useless and doesn't provide anything. Or perhaps it's enough to mark it no-std? ref: https://docs.rust-embedded.org/book/intro/no-std.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After trying a couple of methods, it seems that to avoid the linking issues we would have to at least modify our dependencies (in combination with using no-std).
We can't avoid linking with librust_combined as long as we have the test/boost/rust_test.
We can't merge the mentioned rust wasmtime patch before this one, because similarly, we'll have linking issues.
Having that in mind, I think it would be best if we combined these 2 patches in one PR. I'll soon publish the new PR depending on this one anyway, but recently we decided that there's something I still need to add to it, so you'll have to wait a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also fine with temporarily dropping rust_test if it helps, we can bring it back once we have wasmtime, or just drop it in favor of some custom wasmtime tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see you commented this before yesterday's patch. Would you still like rust_test dropped or will it be fine if we just combine this series with the wasmtime one and try to merge them at the same time?

Copy link
Contributor

Choose a reason for hiding this comment

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

if dropping the test means that we can cheaply avoid disabling wasm-based udfs for a while (even within the same series), then I think it's worth it. But if it's in any way not trivial, the current state is acceptable too, because wasm support is experimental anyway.


if has_wasmtime:
if platform.machine() == 'aarch64':
Expand Down Expand Up @@ -1832,8 +1836,13 @@ def configure_abseil(build_dir, mode, mode_config):
rule unified
command = unified/build_unified.sh --mode $mode --unified-pkg $out
rule rust_header
command = cxxbridge $in > $out
command = cxxbridge --include rust/cxx.h --header $in > $out
description = RUST_HEADER $out
rule rust_source
command = cxxbridge --include rust/cxx.h $in > $out
description = RUST_SOURCE $out
rule cxxbridge_header
command = cxxbridge --header > $out
''').format(**globals()))
for mode in build_modes:
modeval = modes[mode]
Expand Down Expand Up @@ -1893,7 +1902,7 @@ def configure_abseil(build_dir, mode, mode_config):
pool = console
description = TEST {mode}
rule rust_lib.{mode}
command = CARGO_HOME=build/{mode}/rust/.cargo cargo build --release --manifest-path=rust/Cargo.toml --target-dir=build/{mode}/rust -p ${{pkg}}
command = CARGO_HOME=build/{mode}/.cargo cargo build --manifest-path=rust/Cargo.toml --target-dir=build/{mode} --profile=rust-{mode}
description = RUST_LIB $out
''').format(mode=mode, antlr3_exec=antlr3_exec, fmt_lib=fmt_lib, test_repeat=test_repeat, test_timeout=test_timeout, **modeval))
f.write(
Expand All @@ -1912,7 +1921,6 @@ def configure_abseil(build_dir, mode, mode_config):
ragels = {}
antlr3_grammars = set()
rust_headers = {}
rust_libs = {}
seastar_dep = '$builddir/{}/seastar/libseastar.a'.format(mode)
seastar_testing_dep = '$builddir/{}/seastar/libseastar_testing.a'.format(mode)
for binary in sorted(build_artifacts):
Expand All @@ -1926,6 +1934,7 @@ def configure_abseil(build_dir, mode, mode_config):
if has_wasmtime:
objs.append('/usr/lib64/libwasmtime.a')
has_thrift = False
has_rust = False
for dep in deps[binary]:
if isinstance(dep, Thrift):
has_thrift = True
Expand All @@ -1935,8 +1944,9 @@ def configure_abseil(build_dir, mode, mode_config):
if isinstance(dep, Json2Code):
objs += dep.objects('$builddir/' + mode + '/gen')
if dep.endswith('/src/lib.rs'):
lib = dep.replace('/src/lib.rs', '.a').replace('rust/','lib')
objs.append('$builddir/' + mode + '/rust/release/' + lib)
has_rust = True
obj = dep.replace('/src/lib.rs', '.o').replace('rust/','')
objs.append('$builddir/' + mode + '/gen/rust/' + obj)
if binary.endswith('.a'):
f.write('build $builddir/{}/{}: ar.{} {}\n'.format(mode, binary, mode, str.join(' ', objs)))
else:
Expand All @@ -1946,6 +1956,8 @@ def configure_abseil(build_dir, mode, mode_config):
'abseil/' + x for x in abseil_libs
]])
objs.append('$builddir/' + mode + '/gen/utils/gz/crc_combine_table.o')
if has_rust:
objs.append('$builddir/' + mode +'/rust-' + mode + '/librust_combined.a')
if binary in tests:
local_libs = '$seastar_libs_{} $libs'.format(mode)
if binary in pure_boost_tests:
Expand Down Expand Up @@ -1988,8 +2000,6 @@ def configure_abseil(build_dir, mode, mode_config):
elif src.endswith('/src/lib.rs'):
hh = '$builddir/' + mode + '/gen/' + src.replace('/src/lib.rs', '.hh')
rust_headers[hh] = src
staticlib = src.replace('rust/', '$builddir/' + mode + '/rust/release/lib').replace('/src/lib.rs', '.a')
rust_libs[staticlib] = src
else:
raise Exception('No rule for ' + src)
compiles['$builddir/' + mode + '/gen/utils/gz/crc_combine_table.o'] = '$builddir/' + mode + '/gen/utils/gz/crc_combine_table.cc'
Expand Down Expand Up @@ -2035,6 +2045,7 @@ def configure_abseil(build_dir, mode, mode_config):
gen_headers += list(serializers.keys())
gen_headers += list(ragels.keys())
gen_headers += list(rust_headers.keys())
gen_headers.append('$builddir/{}/gen/rust/cxx.h'.format(mode))
gen_headers_dep = ' '.join(gen_headers)

for obj in compiles:
Expand All @@ -2058,10 +2069,13 @@ def configure_abseil(build_dir, mode, mode_config):
for hh in rust_headers:
src = rust_headers[hh]
f.write('build {}: rust_header {}\n'.format(hh, src))
for lib in rust_libs:
src = rust_libs[lib]
package = src.replace('/src/lib.rs', '').replace('rust/','')
f.write('build {}: rust_lib.{} {}\n pkg = {}\n'.format(lib, mode, src, package))
cc = hh.replace('.hh', '.cc')
f.write('build {}: rust_source {}\n'.format(cc, src))
obj = cc.replace('.cc', '.o')
f.write('build {}: cxx.{} {}\n'.format(obj, mode, cc))
f.write('build {}: cxxbridge_header\n'.format('$builddir/{}/gen/rust/cxx.h'.format(mode)))
librust = '$builddir/{}/rust-{}/librust_combined.a'.format(mode, mode)
f.write('build {}: rust_lib.{} {}\n'.format(librust, mode, str.join(' ', rusts)))
for thrift in thrifts:
outs = ' '.join(thrift.generated('$builddir/{}/gen'.format(mode)))
f.write('build {}: thrift.{} {}\n'.format(outs, mode, thrift.source))
Expand Down
44 changes: 28 additions & 16 deletions docs/dev/rust.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,44 @@ shows how to use them in Scylla.

To create a Rust package `new_pkg` and use it in a C++ source file:
1. Create a new package in the `rust` directory using `cargo new new_pkg --lib`
2. Modify crate type to a static library by adding
2. Add `new_pkg = { path = "new_pkg", version = "0.1.0" }` to the dependencies list in `rust/Cargo.toml`
3. Add `extern crate new_pkg;` to `rust/src/lib.rs`
4. Configure your package in `new_pkg/Cargo.toml` and write your Rust code in `new_pkg/src/lib.rs` (and other `new_pkg/src/*.rs` files)
5. To export a function `fn foo(x: i32) -> i32` in the namespace `xyz`, add its declaration to `new_pkg/src/lib.rs` as follows:
```
[lib]
crate-type = ["staticlib"]
```
to `new_pkg/Cargo.toml`
3. Add `"new_pkg",` to the workspace members list in `Cargo.toml`
4. Write your Rust code in `new_pkg/src/lib.rs`
5. To export a function `fn foo(x: i32) -> i32`, add its declaration as follows to the same file
```
#[cxx::bridge(namespace = "rust")]
#[cxx::bridge(namespace = "xyz")]
mod ffi {
extern "Rust" {
fn inc(x: i32) -> i32;
}
}
```
6. Add `"rust/new_pkg/src/lib.rs"` to the `rusts` list in `configure.py`
7. Include the `rust/new_pkg.hh` header and use `rust::foo()` in the selected c++ file.
6. Add source files of the new package to the `rusts` list in `configure.py`
7. Add `new_pkg/src/lib.rs` to the `configure.py` dependencies where you'll need the Rust exports
8. Include the `rust/new_pkg.hh` header and use `xyz::foo()` in the selected c++ file.

## Submitting changes

Additionally to the source code, Scylla tracks the Cargo.lock file that contains precise information about dependency
versions used in the last successful build. Dependency modification may include:
* adding a new local package (it is used in Scylla as a dependency)
* updating a dependency version in an existing package
* adding a new dependency to a package

After each such modification, a new Cargo.lock file should be submitted. Cargo.lock can be generated
by the `cargo update` command. It will also update after each build that modifies the dependencies.

## Rust interoperability implementation

Using Rust alongside C++ in scylla is made possible by the Rust crate CXX. The `cxx::bridge` macro,
together with `mod ffi` and `extern "Rust"` mark items to be exported to C++. During compilation,
a compatible version is generated with temporary names. It can be later linked with a header file, generated from the source code using `cxxbridge` command. The header exposed all items with original names and can be included like any other c++ header.
together with `mod ffi` and `extern "Rust"` mark items to be exported to C++. During compilation
of Rust files, a static library using C++ ABI is generated. The library exports Rust methods under
special names. These names are used in the implementations of C++ methods with the original names.
The C++ methods are listed in *.hh files, and their implementations in *.cc files, both generated
from the Rust source code using `cxxbridge` command.
The header exposes all items with original names and can be included like any other C++ header.

Compilation is managed by `cargo`. Like in any Rust project, modules added to Scylla can be fully
customized using corresponding `Cargo.toml` files. Currently, all modules are compiled to static
libraries (TODO: dynamic or flexible)
customized using corresponding `Cargo.toml` files. All modules are compiled to single static
library, as this is the only officially supported way of linking Rust against C++.
In the future, more linking methods may become supported, possibly using `rlib` ("Rust library") files.
96 changes: 96 additions & 0 deletions rust/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 40 additions & 4 deletions rust/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,40 @@
[workspace]
members = [
"inc",
]
[package]
name = "rust_combined"
version = "0.1.0"
edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
inc = { path = "inc", version = "0.1.0" }

[lib]
crate-type = ["staticlib"]

# The profiles below are Scylla specific. The cargo profile "rust-xyz" should
# be used together with the ninja "xyz" mode.
[profile.rust-dev]
inherits = "dev"
opt-level = 2
debug = false
overflow-checks = false
debug-assertions = false
strip = "symbols"

[profile.rust-debug]
inherits = "dev"
opt-level = 1
incremental = false

[profile.rust-sanitize]
inherits = "dev"
opt-level = "s"
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "opt-level=s" do?

Copy link
Contributor

Choose a reason for hiding this comment

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

binary size: https://doc.rust-lang.org/cargo/reference/profiles.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does "sanitize" mode specifically needs to optimize for size? I'm not saying it's bad, it just sounds strange.

Copy link
Contributor

Choose a reason for hiding this comment

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

The cpp files are compiled with the -Os flag in the sanitize mode, so I suggested using an equivalent flag for rust files.

As for why we are optimizing for size in the cpp files - I'm not sure. The original patchset which introduces sanitize mode (https://groups.google.com/g/scylladb-dev/c/wQ_pw5zkJIg/m/bHW-oeO3AwAJ) doesn't seem to explain it, and the author doesn't work with us anymore. If you have a good argument not to optimize for size here, then I'm completely fine with using a different mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm fine with this optimization mode. Any other choice will seem equality arbitrary...

incremental = false

[profile.rust-release]
inherits = "release"
debug = true

[profile.rust-coverage]
inherits = "dev"
opt-level = 1
incremental = false
5 changes: 1 addition & 4 deletions rust/inc/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,4 @@ edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
cxx = "1.0"

[lib]
crate-type = ["staticlib"]
cxx = "1.0.69"
1 change: 1 addition & 0 deletions rust/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
extern crate inc;