Code refactoring and performance boosts #3

redactedontop · 2025-03-16T19:16:36Z

Howdy!

I've refactored most of your code (in a day! turns out it's exhausting xD), and here are most of the things I did:

Switched to the 2024 rust edition
Removed useless impl's and slow code from Tag
Optimized add_ref and clear_for_drop majorly
Made a lot of style changes

Once I have some time, I ask you to review this code and, for consistency, try to change what I missed to the new code style (as long as you like it). Feel free to propose any changes!

Thanks,
Alex <3

wvwwvwwv · 2025-03-17T06:08:45Z

Awesome!

I'll review the change possibly by next week. In the meantime, could you please give me some performance numbers? Thanks a lot!

redactedontop · 2025-03-18T18:22:45Z

Hey!

Hell yeah! Give me some time (as it's exam season), and I'll get back to you ASAP.

Thanks,
Alex <3

redactedontop · 2025-03-23T13:30:50Z

Hey, sorry!

No benches yet, will try today (on my decent computer).

Thanks,
Alex <3

redactedontop · 2025-04-04T13:04:56Z

Main branch results:

                        change: [-9.3250% -6.7385% -3.7397%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 18 outliers among 100 measurements (18.00%)
  5 (5.00%) high mild
  13 (13.00%) high severe

EBR: superposed guard   time:   [490.25 ps 496.28 ps 502.93 ps]
                        change: [-2.2388% +0.0442% +2.1496%] (p = 0.97 > 0.05)
                        No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild```

redactedontop · 2025-04-04T13:06:06Z

My results:

                        change: [-11.632% -9.5819% -7.8764%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
  5 (5.00%) high mild
  6 (6.00%) high severe

EBR: superposed guard   time:   [719.66 ps 726.75 ps 734.34 ps]
                        change: [+66.492% +69.035% +71.453%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe```
  
  Curious why the regression happens.

redactedontop · 2025-04-04T13:09:30Z

Turns out it's LTO.

Will look back after and check if I can avoid the regression.

…yed-dealloc

redactedontop · 2025-04-04T19:39:06Z

Have refactored the code a bit more. Please take a look at the updated code. I've reduced boilerplate with the 2 Atomic types by using the type-state pattern.

redactedontop · 2025-04-04T19:39:39Z

Another approach would be to use traits, which could produce cleaner code, but I'm not sure I'm ready for the task yet.

redactedontop · 2025-04-04T19:39:58Z

As the Epoch API has changed, I bumped the major version.

redactedontop · 2025-04-04T19:40:59Z

Benchmarks (no LTO):

EBR: guard - time: [6.3678 ns 6.4250 ns 6.5398 ns]
Found 17 outliers among 100 measurements (17.00%)
  9 (9.00%) high mild
  8 (8.00%) high severe

EBR: superposed guard - time: [462.72 ps 466.61 ps 470.89 ps]
Found 5 outliers among 100 measurements (5.00%)
  2 (2.00%) high mild
  3 (3.00%) high severe```

wvwwvwwv · 2025-04-10T18:55:16Z

Hi, thanks for the updates. I really didn't have a chance to look into this PR, but I'll take some time next week.
-> In the meantime, I'll approve workflow runs.

redactedontop · 2025-04-11T12:06:08Z

Hey, can you please approve new runs? I've fixed the issue with the old ones.

redactedontop · 2025-04-13T08:51:48Z

MSRV needs bump. Could you do it pre-merge? I can't figure out which version to set it to.

wvwwvwwv

Hi, I only reviewed half of the change. I'll review the rest by the end of this week.

wvwwvwwv · 2025-04-15T08:36:20Z

Cargo.toml


 [dev-dependencies]
-criterion = "0.5"
+criterion = "0.5.1"


I intentionally omitted patch versions in Cargo.toml - revert it to "0.5".

wvwwvwwv · 2025-04-15T08:36:34Z

Cargo.toml

-[dependencies]
-loom = { version = "0.7", optional = true }
+[dependencies.loom]
+version = "0.7.2"


Fair. I also need to make it optional.

src/atomic.rs

wvwwvwwv · 2025-04-15T08:38:00Z

src/atomic.rs

+pub(super) mod ownership {
+    use crate::ref_counted::RefCounted;
+
+    pub(super) trait Type {


(nit, optional) Can you add doc to each method/type?

I see why it's important but honestly I'm wayyy too lazy to... could you take a shot at it?

wvwwvwwv · 2025-04-15T08:40:08Z

src/collectible.rs

-        if let Some(f) = self.f.take() {
-            f();
-        }
+        let Some(f) = self.f.take() else {


This bumps MSRV, and I don't see any readability improvements here. Can you revert it?

We're bumping MSRV anyway (because of atomic add for it's performance), and I believe this is included as well.

wvwwvwwv · 2025-04-15T08:42:39Z

src/collector.rs

-                }
-            }
-        } else {
+        if (*collector_ptr).num_readers > 0 {


Any justification for this refactoring? Comparing against 0 is "usually" better than '>' in terms of performance.

The compiler can most probably optimize it away, and it causes much cleaner code.

src/collector.rs

wvwwvwwv · 2025-04-15T08:45:33Z

src/collector.rs

        let mut current = GLOBAL_ROOT.chain_head.load(Relaxed);
-        loop {
+
+        unsafe {


This repeats line 244-248. Can you revert it?

It's either repeating or some ugly nesting, I thought this would be preferred.

src/collector.rs

wvwwvwwv · 2025-04-15T08:48:46Z

src/collector.rs

-                    current_collector_ptr = (*collector_ptr).next_link.load(Relaxed);
-                    continue;
-                }
+        let Ok(mut current_ptr) = Self::lock_chain() else {


This is debatable; your code is clearly "clearer", but this bumps MSRV. I'll need to think about the pros and cons here.
-> Since you bumped SDD to 4, we can make breaking changes, though keeping MSRV as low as possible is good for dependent libraries.

See the previous comment about let-else.

wvwwvwwv · 2025-04-15T08:50:27Z

MSRV needs bump. Could you do it pre-merge? I can't figure out which version to set it to.

You can test it with cargo msrv verify; the lower the better!

wvwwvwwv · 2025-04-15T15:20:21Z

src/tag.rs

-            Tag::Second => 2,
-            Tag::Both => 3,
-        }
+    fn not(self) -> usize {


Why is it needed? What is the semantics of this method? A Tag is intentionally a four-state type, but the output of ! is a usize that doesn't quite make sense to me...

Can you explain further?

wvwwvwwv · 2025-04-15T15:21:11Z

src/tag.rs

-            2 => Ok(Tag::Second),
-            3 => Ok(Tag::Both),
-            _ => Err(val),
+    fn add(self, rhs: Self) -> Tag {


Tag is supposed to contain either of four possible values; this doesn't seem right.

Please explain further as well: this is used for later code replacing a match statement with arithmetic (just like the previous comment!).

src/tag.rs

wvwwvwwv · 2025-04-15T15:22:19Z

src/tag.rs


 /// [`Tag`] is a four-state `Enum` that can be embedded in a pointer as the two least
 /// significant bits of the pointer value.
+#[repr(usize)]


usize is a waste of memory for a four-state type.

Improves performance as you don't need to convert an u8 to a usize, even though I believe that'll be optimized away by the compiler.

wvwwvwwv · 2025-04-15T15:24:56Z

src/ref_counted.rs

    pub(super) fn drop_ref(&self) -> bool {
        // It does not have to be a load-acquire as everything's synchronized via the global
        // epoch.
        let mut current = self.ref_cnt().load(Relaxed);


Thanks!

Could you revive the debug assertion?

Would be hard without messing with the code a lot.

wvwwvwwv · 2025-04-15T15:26:01Z

src/ref_counted.rs

-                    }
-                },
-            )
+            .fetch_update(order, order, |r| (r & 1 == 1).then_some(r + 2))


wvwwvwwv · 2025-04-15T15:26:55Z

src/owned.rs

-    pub fn get_guarded_ref<'g>(&self, _guard: &'g Guard) -> &'g T {
-        unsafe { std::mem::transmute::<&T, _>(&**self) }
+    pub fn get_guarded_ref<'g>(&self, _: &'g Guard) -> &'g T {
+        #[allow(clippy::missing_transmute_annotations)]


Would you not omit type annotations here?

wvwwvwwv · 2025-04-15T15:28:25Z

src/model.rs

@@ -0,0 +1,97 @@
+#[cfg(feature = "loom")]


Please remove this file.

Will do tomorrow.

wvwwvwwv · 2025-04-15T15:32:51Z

src/epoch.rs

    /// let prev_prev = prev.prev();
    /// assert!(prev_prev < prev);
    /// ```
+    #[allow(clippy::precedence)]


Could you just add a parenthesis instead of suppressing the error?

Well thought of.

src/epoch.rs

wvwwvwwv · 2025-08-19T18:59:38Z

Hi, any chance to update this PR?

loyd · 2025-09-30T15:53:18Z

intel i9-14900, no difference:

group                    main                                   pr3
-----                    ----                                   ---
EBR: guard               1.00      6.0±0.02ns        ? ?/sec    1.00      6.0±0.01ns        ? ?/sec
EBR: superposed guard    1.00      0.4±0.00ns        ? ?/sec    1.00      0.4±0.00ns        ? ?/sec

loyd · 2025-09-30T15:53:41Z

and 3rd major update in one year =(

redactedontop · 2025-11-19T16:41:33Z

intel i9-14900, no difference:

group                    main                                   pr3
-----                    ----                                   ---
EBR: guard               1.00      6.0±0.02ns        ? ?/sec    1.00      6.0±0.01ns        ? ?/sec
EBR: superposed guard    1.00      0.4±0.00ns        ? ?/sec    1.00      0.4±0.00ns        ? ?/sec

Compile with target-cpu.

redactedontop · 2025-11-19T16:41:40Z

Hi, any chance to update this PR?

Soon

Code refactoring and performance boosts

5753b84

redactedontop added 3 commits April 4, 2025 14:48

Remove LTO by default as it regresses on one of the benchmarks

5b4e587

Will look back after and check if I can avoid the regression.

Round 2 of code changes + used type-state pattern for both Atomic types.

ae44e55

Merge branch 'main' of https://github.com/redactedontop/scalable-dela…

a0dc31f

…yed-dealloc

redactedontop added 3 commits April 5, 2025 08:16

Cleaned up Drop block in Atomic

c064f22

Minor cleanup

341a985

Inlined trait functions

a7ce537

Removed const from functions.

fbda955

wvwwvwwv reviewed Apr 15, 2025

View reviewed changes

wvwwvwwv force-pushed the main branch from f74900b to 3853267 Compare August 19, 2025 19:00

Code refactoring and performance boosts #3

Are you sure you want to change the base?

Code refactoring and performance boosts #3

Uh oh!

Conversation

redactedontop commented Mar 16, 2025

Uh oh!

wvwwvwwv commented Mar 17, 2025

Uh oh!

redactedontop commented Mar 18, 2025

Uh oh!

redactedontop commented Mar 23, 2025

Uh oh!

redactedontop commented Apr 4, 2025

Uh oh!

redactedontop commented Apr 4, 2025

Uh oh!

redactedontop commented Apr 4, 2025

Uh oh!

redactedontop commented Apr 4, 2025

Uh oh!

redactedontop commented Apr 4, 2025

Uh oh!

redactedontop commented Apr 4, 2025

Uh oh!

redactedontop commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wvwwvwwv commented Apr 10, 2025

Uh oh!

redactedontop commented Apr 11, 2025

Uh oh!

redactedontop commented Apr 13, 2025

Uh oh!

wvwwvwwv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvwwvwwv commented Apr 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

redactedontop commented Apr 4, 2025 •

edited

Loading