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

Add #[inline] to short public functions #243

Merged
merged 5 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/helper/color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ pub fn split_rgb(rgb: &str) -> (i32, i32, i32) {
(r, g, b)
}

#[inline]
pub fn join_rgb(r: &i32, g: &i32, b: &i32) -> String {
format!("{:02X}{:02X}{:02X}", r, g, b)
}
Expand Down Expand Up @@ -174,6 +175,7 @@ pub fn set_color(t1: &f64, t2: &f64, t3: &f64) -> f64 {
color
}

#[inline]
fn positive_decimal_part(hue: &f64) -> f64 {
let hue = hue % 1.0;

Expand All @@ -183,6 +185,7 @@ fn positive_decimal_part(hue: &f64) -> f64 {
1.0 + hue
}

#[inline]
fn to_i32(num: f64) -> i32 {
num.round() as i32
}
11 changes: 11 additions & 0 deletions src/helper/coordinate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ where
.sum::<u32>()
}

#[inline]
pub fn column_index_from_string<S: AsRef<str>>(column: S) -> u32 {
let column_c = column.as_ref();
if column_c == "0" {
Expand All @@ -51,6 +52,7 @@ pub fn column_index_from_string<S: AsRef<str>>(column: S) -> u32 {
alpha_to_index(column_c)
}

#[inline]
pub fn string_from_column_index(column_index: &u32) -> String {
assert!(column_index >= &1u32, "Column number starts from 1.");

Expand Down Expand Up @@ -91,6 +93,7 @@ where
.unwrap_or_default()
}

#[inline]
pub fn coordinate_from_index(col: &u32, row: &u32) -> String {
format!("{}{}", string_from_column_index(col), row)
}
Expand All @@ -110,6 +113,7 @@ pub fn coordinate_from_index_with_lock(
)
}

#[inline]
pub(crate) fn adjustment_insert_coordinate(num: &u32, root_num: &u32, offset_num: &u32) -> u32 {
if (num >= root_num && offset_num != &0) {
num + offset_num
Expand All @@ -118,6 +122,7 @@ pub(crate) fn adjustment_insert_coordinate(num: &u32, root_num: &u32, offset_num
}
}

#[inline]
pub(crate) fn adjustment_remove_coordinate(num: &u32, root_num: &u32, offset_num: &u32) -> u32 {
if (num >= root_num && offset_num != &0) {
num - offset_num
Expand All @@ -126,6 +131,7 @@ pub(crate) fn adjustment_remove_coordinate(num: &u32, root_num: &u32, offset_num
}
}

#[inline]
pub(crate) fn is_remove_coordinate(num: &u32, root_num: &u32, offset_num: &u32) -> bool {
if root_num != &0 && offset_num != &0 {
return num >= root_num && num < &(root_num + offset_num);
Expand All @@ -143,31 +149,36 @@ pub struct CellCoordinates {
}

impl CellCoordinates {
#[inline]
fn new(col: u32, row: u32) -> Self {
CellCoordinates { row, col }
}
}

impl From<(u32, u32)> for CellCoordinates {
#[inline]
fn from(value: (u32, u32)) -> Self {
CellCoordinates::new(value.0, value.1)
}
}

impl From<(&u32, &u32)> for CellCoordinates {
#[inline]
fn from(value: (&u32, &u32)) -> Self {
CellCoordinates::new(*value.0, *value.1)
}
}

impl From<String> for CellCoordinates {
#[inline]
fn from(value: String) -> Self {
let str_ref: &str = value.as_ref();
str_ref.into()
}
}

impl From<&str> for CellCoordinates {
#[inline]
fn from(value: &str) -> Self {
let (col, row, ..) = index_from_coordinate(value.to_uppercase());
CellCoordinates::new(col.unwrap(), row.unwrap())
Expand Down
8 changes: 8 additions & 0 deletions src/helper/crypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -477,25 +477,29 @@ fn hash(algorithm: &str, buffers: Vec<&[u8]>) -> Result<Vec<u8>, String> {
Ok(digest.finalize().to_vec())
}

#[inline]
fn gen_random_16() -> Vec<u8> {
let buf: &mut [u8] = &mut [0; 16];
getrandom::getrandom(buf);
buf.to_vec()
}

#[inline]
fn gen_random_32() -> Vec<u8> {
let buf: &mut [u8] = &mut [0; 32];
getrandom::getrandom(buf);
buf.to_vec()
}

#[inline]
fn gen_random_64() -> Vec<u8> {
let buf: &mut [u8] = &mut [0; 64];
getrandom::getrandom(buf);
buf.to_vec()
}

// Create a buffer of an integer encoded as a uint32le
#[inline]
Copy link

Choose a reason for hiding this comment

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

Are we sure all of these are more suitable to be inline? Did we check what impact this has on binary size?

Copy link

Choose a reason for hiding this comment

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

To be fair I haven't done deep research on using inline. Most of my reading is from this article https://matklad.github.io/2021/07/09/inline-in-rust.html and I'm just wondering if this incurs more compile times for all users even if they would have preferred faster compiles with possibly slightly slower code.

Copy link
Contributor Author

@schungx schungx Nov 20, 2024

Choose a reason for hiding this comment

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

Putting #[inline] on functions enable them to be optimized across crates. This is a library crate, so if you don't have that and LTO is not on, the Rust compiler will never inline even very simple function such as getters.

Also, inlining usually enables more optimizations in the call site. For example, the following code:

pub fn flip(x: bool) -> bool {
    if x { false } else { true }
}

// use it in another crate:
if flip(true) { ... }

If you don't compile with LTO, everything stays, including the call to flip because the Rust compiler will not inline across crate boundaries even in release builds. If you put #[inline] on fn flip, the entire code is optimized away to zero.

That's why if you look at Rust standard library code, it is littered with #[inline] all over the place. In fact, it is better to err on the side of having too many #[inline] because the compiler knows when to stop inlining if it thinks it is not worth it. The default of inlining of any function for LLVM is "always inline".

On to your question: inlining reduces binary size for short functions, because you lose all those code for setting up the function call and the tear-down, as well as the code overhead of setting up a call frame in the function. It does not affect compilation time in debug builds because Rust does not inline in debug. With release builds, my experience is that the added compilation time is immaterial compared to all the other optimizations Rust is doing. And I suppose if a user builds for release, he/she would want execution speed, not compilation speed.

Finally, these attributes is one of those things that are very easy to add when you first create the API, but extremely tedious and time-consuming to add back later on, so it pays to put them in on day 1.

Copy link

@c-git c-git Nov 20, 2024

Choose a reason for hiding this comment

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

There is a cost in terms of compile time by doing that optimization. All I was saying is that by doing this we remove that choice from the end user. Whereas with LTO they can opt in if they want to. If you say it doesn't affect debug then I guess that should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK Rust doesn't inline at all in debug mode. That's why performance is way slower than release, because all those as_ref and as_mut etc are actually function calls where with inlining they turn to the original reference.

Copy link
Contributor Author

@schungx schungx Nov 20, 2024

Choose a reason for hiding this comment

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

Fourth, when building libraries, proactively add #[inline] to small non-generic functions. Pay special attention to impls: Deref, AsRef and the like often benefit from inlining. A library can’t anticipate all usages upfront, it makes sense to not prematurely pessimize future users. Note that #[inline] is not transitive: if a trivial public function calls a trivial private function, you need to #[inline] both.

This section is actually my exact point. A library should proactively #[inline] wherever it makes sense.

fn create_uint32_le_buffer(value: &u32, buffer_size: Option<&usize>) -> Vec<u8> {
let bs_prm = buffer_size.unwrap_or(&4);
let mut buffer = buffer_alloc(0, *bs_prm);
Expand Down Expand Up @@ -631,10 +635,12 @@ fn build_encryption_info(
buffer_concat(vec![&ENCRYPTION_INFO_PREFIX.to_vec(), &result])
}

#[inline]
fn buffer_slice(buffer: &[u8], start: usize, end: usize) -> Vec<u8> {
buffer[start..end].to_vec()
}

#[inline]
fn buffer_alloc(alloc_char: u8, size: usize) -> Vec<u8> {
vec![alloc_char; size]
}
Expand All @@ -652,10 +658,12 @@ fn buffer_copy(buffer1: &mut [u8], buffer2: &[u8]) {
}
}

#[inline]
fn buffer_read_u_int32_le(buffer: &[u8], _cnt: &usize) -> u32 {
LittleEndian::read_u32(buffer)
}

#[inline]
fn buffer_write_u_int32_le(buffer: &mut [u8], value: &u32, _cnt: &usize) {
LittleEndian::write_u32(buffer, *value);
}
Expand Down
4 changes: 4 additions & 0 deletions src/helper/date.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,12 @@ pub fn excel_to_date_time_object(
+ Duration::seconds(seconds as i64)
}

#[inline]
fn get_default_timezone() -> String {
String::from("UTC")
}

#[inline]
pub fn convert_date(
year: i32,
month: i32,
Expand All @@ -54,6 +56,7 @@ pub fn convert_date(
convert_date_windows_1900(year, month, day, hours, minutes, seconds)
}

#[inline]
pub fn convert_date_windows_1900(
year: i32,
month: i32,
Expand All @@ -65,6 +68,7 @@ pub fn convert_date_windows_1900(
convert_date_crate(year, month, day, hours, minutes, seconds, true)
}

#[inline]
pub fn convert_date_mac_1904(
year: i32,
month: i32,
Expand Down
7 changes: 7 additions & 0 deletions src/helper/formula.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ pub struct FormulaToken {
token_sub_type: FormulaTokenSubTypes,
}
impl Default for FormulaToken {
#[inline]
fn default() -> Self {
Self {
value: StringValue::default(),
Expand All @@ -56,28 +57,34 @@ impl Default for FormulaToken {
}
}
impl FormulaToken {
#[inline]
pub fn get_value(&self) -> &str {
self.value.get_value_str()
}

#[inline]
pub fn set_value<S: Into<String>>(&mut self, value: S) -> &mut Self {
self.value.set_value(value);
self
}

#[inline]
pub fn get_token_type(&self) -> &FormulaTokenTypes {
&self.token_type
}

#[inline]
pub fn set_token_type(&mut self, value: FormulaTokenTypes) -> &mut Self {
self.token_type = value;
self
}

#[inline]
pub fn get_token_sub_type(&self) -> &FormulaTokenSubTypes {
&self.token_sub_type
}

#[inline]
pub fn set_token_sub_type(&mut self, value: FormulaTokenSubTypes) -> &mut Self {
self.token_sub_type = value;
self
Expand Down
14 changes: 14 additions & 0 deletions src/helper/html.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use thin_vec::ThinVec;
/// .get_alignment_mut()
/// .set_wrap_text(true);
/// ```
#[inline]
pub fn html_to_richtext(html: &str) -> Result<RichText, html_parser::Error> {
html_to_richtext_custom(html, &DataAnalysis::default())
}
Expand All @@ -39,6 +40,7 @@ pub fn html_to_richtext(html: &str) -> Result<RichText, html_parser::Error> {
/// * `method` - struct for analysis.
/// # Return value
/// * `Result<RichText, html_parser::Error>`
#[inline]
pub fn html_to_richtext_custom(
html: &str,
method: &AnalysisMethod,
Expand Down Expand Up @@ -181,17 +183,20 @@ pub struct HfdElement {
classes: ThinVec<String>,
}
impl HfdElement {
#[inline]
pub fn has_name(&self, name: &str) -> bool {
self.name == name
}

#[inline]
pub fn get_by_name_and_attribute(&self, name: &str, attribute: &str) -> Option<&str> {
self.attributes
.get(attribute)
.and_then(|v| (self.name == name).then(|| v))
.map(|x| x.as_str())
}

#[inline]
pub fn contains_class(&self, class: &str) -> bool {
self.classes.contains(&class.to_string())
}
Expand All @@ -213,13 +218,15 @@ pub trait AnalysisMethod {
#[derive(Clone, Default, Debug)]
struct DataAnalysis {}
impl AnalysisMethod for DataAnalysis {
#[inline]
fn font_name<'a>(&'a self, html_flat_data: &'a HtmlFlatData) -> Option<&str> {
html_flat_data
.element
.iter()
.find_map(|element| element.get_by_name_and_attribute("font", "face"))
}

#[inline]
fn size(&self, html_flat_data: &HtmlFlatData) -> Option<f64> {
html_flat_data.element.iter().find_map(|element| {
element
Expand All @@ -245,33 +252,40 @@ impl AnalysisMethod for DataAnalysis {
})
}

#[inline]
fn is_tag(&self, html_flat_data: &HtmlFlatData, tag: &str) -> bool {
html_flat_data
.element
.iter()
.any(|element| element.has_name(tag))
}

#[inline]
fn is_bold(&self, html_flat_data: &HtmlFlatData) -> bool {
self.is_tag(html_flat_data, "b") || self.is_tag(html_flat_data, "strong")
}

#[inline]
fn is_italic(&self, html_flat_data: &HtmlFlatData) -> bool {
self.is_tag(html_flat_data, "i") || self.is_tag(html_flat_data, "em")
}

#[inline]
fn is_underline(&self, html_flat_data: &HtmlFlatData) -> bool {
self.is_tag(html_flat_data, "u") || self.is_tag(html_flat_data, "ins")
}

#[inline]
fn is_superscript(&self, html_flat_data: &HtmlFlatData) -> bool {
self.is_tag(html_flat_data, "sup")
}

#[inline]
fn is_subscript(&self, html_flat_data: &HtmlFlatData) -> bool {
self.is_tag(html_flat_data, "sub")
}

#[inline]
fn is_strikethrough(&self, html_flat_data: &HtmlFlatData) -> bool {
self.is_tag(html_flat_data, "del")
}
Expand Down
1 change: 1 addition & 0 deletions src/helper/number_format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub struct Split<'r, 't> {
last: usize,
}

#[inline]
pub fn split<'r, 't>(regex: &'r Regex, text: &'t str) -> Split<'r, 't> {
Split {
finder: regex.find_iter(text),
Expand Down
1 change: 1 addition & 0 deletions src/helper/number_format/fraction_formater.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ pub(crate) fn format_as_fraction(value: &f64, format: &str) -> String {
result
}

#[inline]
fn gcd(a: &f64, b: &f64) -> f64 {
if b == &0f64 {
*a
Expand Down
2 changes: 2 additions & 0 deletions src/helper/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,12 @@ pub fn get_start_and_end_point(range_str: &str) -> (u32, u32, u32, u32) {
(row_start, row_end, col_start, col_end)
}

#[inline]
pub fn get_split_range(range: &str) -> Vec<&str> {
range.split(':').collect()
}

#[inline]
pub fn get_join_range(coordinate_list: &[String]) -> String {
coordinate_list.join(":")
}
3 changes: 3 additions & 0 deletions src/helper/string_helper.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
#[inline]
pub(crate) fn _get_currency_code() -> String {
String::from("")
}

#[inline]
pub(crate) fn _get_decimal_separator() -> String {
String::from(".")
}

#[inline]
pub(crate) fn _get_thousands_separator() -> String {
String::from(",")
}
1 change: 1 addition & 0 deletions src/helper/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
macro_rules! from_err {
($from:ty, $to:tt, $var:tt) => {
impl From<$from> for $to {
#[inline]
fn from(e: $from) -> $to {
$to::$var(e)
}
Expand Down
Loading