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

Sprinkle #[inline] across scylla-cql crate to allow inlining into scylla crate #949

Open
wprzytula opened this issue Mar 6, 2024 · 3 comments
Labels
enhancement New feature or request performance Improves performance of existing features
Milestone

Comments

@wprzytula
Copy link
Collaborator

wprzytula commented Mar 6, 2024

Motivation

Having read the guidelines on inlining (highly recommended for everyone!), I've understood that adding #[inline] explicitly is required to allow inlining across crates.

Even with recent rustc enhancements (related PR), the inlining policy is very conservative at the moment - requires that an (already optimized) function subject to cross-crate inlining does not contain any asserts nor calls.

Problem

scylla-cql crate's functions are largely short ones and, at the same time, are heavily called by scylla crate's routines.

Suggestion

Adding #[inline] to scylla-cql short functions (as already done in e.g. recently added RowSerializationContext) can bring considerable performance benefits on the ser/de code path.

Additionally, we can think about small functions in scylla that could make the driver's users benefit from their inlining.

Note

Do not forget that #[inline] requires transitivity. In the following code:

fn foo(x: i32) -> i32 {
  x * 2
}

#[inline]
pub fn bar(x: i32) -> i32 {
  foo(x) + 1
}

if bar is called from another crate, it won't be inlined (however, IIUC, it's probable that the call to foo() will be inlined into bar body).
In order to enable inlining bar(), it is mandatory* that both foo and bar functions are marked with #[inline].

*not necessarily after the mentioned recent changes to rustc.

@mykaul mykaul added enhancement New feature or request performance Improves performance of existing features labels Mar 6, 2024
@mykaul
Copy link
Contributor

mykaul commented Mar 6, 2024

Do we compile / recommend to compile with LTO?

@Lorak-mmk
Copy link
Collaborator

Lorak-mmk commented Mar 6, 2024

Worth noting: @piodul found out that rustc already cross-crate inlines small functions without asserts / other calls: https://doc.rust-lang.org/stable/releases.html#version-1750-2023-12-28

Do we compile / recommend to compile with LTO?

We don't compile - we have no way to, profile settings for dependencies are ignored.
We don't recommend. Should we? I'm not sure. I think it highly depends on a specific application and it's not specific to the driver at all, so it's not our place to make such recommendations. Also, Rust already gets a lot of bad rep for compile times, so I get why people might be reluctant to enable LTO.

@wprzytula
Copy link
Collaborator Author

Worth noting: @piodul found out that rustc already cross-crate inlines small functions without asserts / other calls: https://doc.rust-lang.org/stable/releases.html#version-1750-2023-12-28

Updated the issue description to mention this.

@wprzytula wprzytula added this to the 1.1.0 milestone May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Improves performance of existing features
Projects
None yet
Development

No branches or pull requests

3 participants