Skip to content

Inline finaliziers - #55

Merged
DoumanAsh merged 1 commit into
DoumanAsh:masterfrom
lukaslueg:inline
Jul 21, 2026
Merged

Inline finaliziers#55
DoumanAsh merged 1 commit into
DoumanAsh:masterfrom
lukaslueg:inline

Conversation

@lukaslueg

Copy link
Copy Markdown
Contributor

This encourages LLVM to inline .digest() into the caller. Currently, while the comments explicitly refer to inlining, LLVM does not inline these methods for all cases I measured, but does with the attribute. Notice that #[inline] remains the weaker suggestion compared to #[inline(always)], but it brings us over the line (pun intended).

Currently, LLVM does not actually take information from surrounding code into account. The benefit of adding #[inline] therefor is mainly around the Hasher-implementation or when not using the explicit one-shot path: LLVM will inline the entire thing into the caller when it then can aggressively remove all the state-handling, usually if the to-be-hashed value has a compile-time-known size. Hashing a T via T: std::hash::Hash where T is u64 goes down by around -67% runtime, [u64; 4] down by -76%; similar small objects mostly the same; longer slices are mostly flat. Think of HashMap<u64, ...> as a beneficiary.

Inlining-changes are always somewhat opaque, so I encourage to benchmark yourself. I tested this change on AArch64 with Rust 1.81, 1.97, ThinLTO enabled/disabled, various codegen-unit settings, and - at least as far as the assembly goes - x86-64. The added hints consistently come with performance benefits especially around small-ish, constant-size objects; LLVM also does not over-aggressively inline e.g. for dynamic buffers. YMMV, but as far as I can see this is a win.

@DoumanAsh

Copy link
Copy Markdown
Owner

I think I initially wanted to avoid inline because internal methods like xxh3_64_internal always use inline suggestion, hence it would potentially mean bloating code if you inline APIs
But well if there can be performance benefit that could be warranted

It is though strange than under LTO it wouldn't at least consider to optimize it on its own...

I will give it some tests myself on this weekend to see how effective it is on my laptop

@lukaslueg

Copy link
Copy Markdown
Contributor Author

That'd be my recommendation. It took embarrassingly many cycles to get a stable read on what LLVM does in different scenarios, but at the end it seems sufficiently stable that LLVM does use inline+dead-code-elimination opportunities, yet does not spill the guts of those functions everywhere

@DoumanAsh

Copy link
Copy Markdown
Owner

I did some runs of my performance tests, but results were hardly consistent so I'm not sure if it was effect of is inlining
Since my benchmark code is very simple and uses diges methods directly it might has less overhead anyway making it easy for LLVM to inline by default when LTO enabled.
Still there seem to be performance effect on adding inline for sure, so there is no reason not to have inline hint and leave decision to compiler

Some samples:

xxh3/stream/64/RustDefault/149b
                        time:   [221.80 ns 221.88 ns 221.96 ns]
                        thrpt:  [640.19 MiB/s 640.43 MiB/s 640.65 MiB/s]
                 change:
                        time:   [−1.0187% −0.8893% −0.7946%] (p = 0.00 < 0.05)
                        thrpt:  [+0.8010% +0.8973% +1.0292%]
                        Change within noise threshold.
xxh3/stream/64/RustDefault/240b
                        time:   [188.60 ns 188.65 ns 188.71 ns]
                        thrpt:  [1.1845 GiB/s 1.1848 GiB/s 1.1851 GiB/s]
                 change:
                        time:   [−2.1767% −2.1266% −2.0728%] (p = 0.00 < 0.05)
                        thrpt:  [+2.1166% +2.1728% +2.2251%]
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe
xxh3/stream/64/RustDefault/2046b
                        time:   [671.58 ns 672.59 ns 673.75 ns]
                        thrpt:  [2.8282 GiB/s 2.8331 GiB/s 2.8373 GiB/s]
                 change:
                        time:   [−0.3166% −0.1617% +0.0306%] (p = 0.09 > 0.05)
                        thrpt:  [−0.0305% +0.1620% +0.3176%]
                        No change in performance detected.
xxh3/stream/128/RustDefault/240b
                        time:   [164.65 ns 165.34 ns 166.14 ns]
                        thrpt:  [1.3453 GiB/s 1.3519 GiB/s 1.3575 GiB/s]
                 change:
                        time:   [−6.4575% −6.2006% −5.9126%] (p = 0.00 < 0.05)
                        thrpt:  [+6.2842% +6.6104% +6.9033%]
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  3 (3.00%) high mild
  4 (4.00%) high severe
xxh3/stream/128/RustDefault/2046b
                        time:   [636.65 ns 636.92 ns 637.21 ns]
                        thrpt:  [2.9904 GiB/s 2.9917 GiB/s 2.9930 GiB/s]
                 change:
                        time:   [−1.1878% −1.0094% −0.8550%] (p = 0.00 < 0.05)
                        thrpt:  [+0.8624% +1.0197% +1.2021%]
                        Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
  4 (4.00%) high mild
  1 (1.00%) high severe

@DoumanAsh
DoumanAsh merged commit c9bd2a1 into DoumanAsh:master Jul 21, 2026
7 checks passed
@lukaslueg
lukaslueg deleted the inline branch July 21, 2026 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants