Guarantee uE implementors always get the correct UUID when calling `UUIDBuilder::build()` by using internally held singleton #74

PLeVasseur · 2024-03-29T18:17:03Z

Addresses #58

What

Updated UUIDBuilder to expose only build() to allow end-users a simpler, more consistent API when generating UUIDs for their uE.

Why

End users of the library should be shielded from making mistakes, which is what we do here. They simply call UUIDBuilder::build() and they'll always get the correct result.

How

We use an internally held instance of UUIDBuilder which generates the rand_b portion once, meaning all calls to build() have the same rand_b in the UUIDs.

We use a compare-and-swap (CAS) technique using an AtomicU64 to ensure we correctly handled logic for generating timestamp + counter portion, even in extremely high-contention scenarios when generating UUIDs. To do so we had to refactor so that we regenerate the timestamp at the top of the CAS loop, rather than be passed in the timestamp to use.

Mutex vs Atomic Compare-and-Swap (CAS) Performance

Using the Atomic CAS technique is approximately twice as fast as using a Mutex

With 10 async tasks, each generating 400 UUIDs all released and run at once

Mutex: ~2ms
Atomic CAS: ~0.8ms

evshary

@PLeVasseur I like your idea of using CAS. I left some nitpicks.

evshary · 2024-03-30T09:13:52Z

src/uattributes/uattributesvalidator.rs

@@ -590,17 +593,22 @@ mod tests {
            ttl,
            ..Default::default()
        };
+
+        if let Some(created_millis_ago) = created_millis_ago {
+            thread::sleep(std::time::Duration::from_millis(created_millis_ago));


This is more intuitive to create a timeout UAttributes, but I'm not sure whether this will need a longer time to do the test. (Maybe it's ok while we don't have too many of this kind of tests).

src/uattributes/uattributesvalidator.rs

src/uuid/uuidbuilder.rs

evshary · 2024-03-30T09:34:03Z

src/uuid/uuidbuilder.rs

-        UUID::from_u64_pair(new_msb, self.lsb)
-            .expect("should have been able to create UUID for valid timestamp")
+            // Try to update the MSB atomically.
+            match self.msb.compare_exchange(


Okay, so we use compare_exchange instead of swap here to ensure no one modifies the self.msb, right?
Brilliant idea!

src/uuid/uuidbuilder.rs

AnotherDaniel

Looks good overall, like what you can do with Rust to support the desired functionality, and that you put in the time to figure it out :)

src/uuid/uuidbuilder.rs

Cargo.toml

src/uuid/uuidbuilder.rs

sophokles73 · 2024-04-02T14:24:42Z

src/uattributes/uattributesvalidator.rs

+        thread::sleep(std::time::Duration::from_millis(created_millis_ago));
+


FMPOV it is an anti-pattern to rely on sleeping in test code. Please replace this with the original or a clock-tick based approach.

I agree that it's bad to sleep in test code. Problem is that we now calculate the timestamp within the build_internal() function. Hmm. Do you have ideas on how to navigate the situation now that's the case?

OK -- I think I was able to pull this off. Can you take another look @sophokles73?

src/uuid/uuidbuilder.rs

…llow end-users a simpler, more consistent API when generating UUIDs for their uE.

…ndencies

…on, we wait for another millisecond to pass.

sophokles73 · 2024-04-03T06:48:14Z

src/uattributes/uattributesvalidator.rs

-
-        let creation_time = now_millis_since_epoch_start - created_millis_ago;
-        UUIDBuilder::new().build_with_instant(creation_time)
+    fn uuid_n_ms_in_past(n_ms_in_past: u64) -> UUID {


Maybe we can add this as pub(crate) fn UUIDBuilder::build_with_instant(timestap) -> UUID so that it can (potentially) be reused in other test cases of the up-rust crate?

I could see adding a pub(crate) function which does what I wrote here -- generates a UUID and then subtracts a certain amount of time from it.

I ran into issues with passing in an absolute timestamp. Read on to hear of my woes 👇

I was running into a fair amount of tricky debugging and headscratching before I moved away from the two-step process that was here of build() -> build_with_timestamp(), to calculating the timestamp purely in the loop of the compare-and-swap and not passing in the timestamp.

Imagine on thread 1 we had entered build_internal() from build_with_timestamp() with a timestamp of n and on thread 2 we had entered build_internal() from build_with_timestamp() with a timestamp of n+1, and the previous time in the msb` was n.

We make it to thread 2 first, see the msb time of n which differs from the passed in timestamp of n+1 and set the timestamp to n+1.

We hit thread 1 second, see the msb time of n+1 which differs from the passed in timestamp of n and set the timestamp to n (going back in time!).

If I missed the point though, please feel free to let me know your thoughts 🙂

I could see adding a pub(crate) function which does what I wrote here -- generates a UUID and then subtracts a certain amount of time from it.

That is what I had in mind :-) Just didn't want to bury it in the test cases ...

😆 Can you tell from my paragraph that I had gone through some trauma...

Sounds good, I'll move this over into UUIDBuilder so it can be accessed from elsewhere for other tests 👍

Okey dokey -- can you take another look?

sophokles73 · 2024-04-03T06:49:12Z

src/uuid/uuidbuilder.rs

+pub(crate) const BITMASK_CLEAR_VERSION: u64 = 0xffff_ffff_ffff_0fff;
+const BITMASK_CLEAR_VARIANT: u64 = 0x3fff_ffff_ffff_ffff;


nice formatting 👍

src/uuid/uuidbuilder.rs

…DBuilder to allow other tests access

sophokles73

LGTM

Thanks for contributing 👍

PLeVasseur force-pushed the feature/uuidbuilder_thread-safe_proposal branch 2 times, most recently from c0be6b9 to 39c25e7 Compare March 29, 2024 18:47

PLeVasseur mentioned this pull request Mar 30, 2024

UPClientZenoh should store the UUIDBuilder and reuse it. eclipse-uprotocol/up-transport-zenoh-rust#12

Closed

evshary reviewed Mar 30, 2024

View reviewed changes

AnotherDaniel reviewed Apr 2, 2024

View reviewed changes

src/uuid/uuidbuilder.rs Outdated Show resolved Hide resolved

src/uuid/uuidbuilder.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

PLeVasseur mentioned this pull request Apr 2, 2024

UUIDBuilder -- should it be a singleton as it is in the other language libs? (e.g. up-java and up-cpp) #58

Closed

sophokles73 added the enhancement New feature or request label Apr 2, 2024

sophokles73 requested changes Apr 2, 2024

View reviewed changes

PLeVasseur added 5 commits April 2, 2024 11:33

Updated UUIDBuilder to be a singleton which exposes only build() to a…

ca6c21d

…llow end-users a simpler, more consistent API when generating UUIDs for their uE.

Updated based on PR feedback

f35576d

Added some comments around asserts and moved logging over to dev depe…

7133844

…ndencies

Updated so that if we'd have a duplicate UUID due to counter saturati…

c1d0145

…on, we wait for another millisecond to pass.

Post-rebase cleanup

9594a1a

PLeVasseur force-pushed the feature/uuidbuilder_thread-safe_proposal branch from 12a72f2 to 9594a1a Compare April 2, 2024 15:45

Update tests to avoid sleep

75ec54c

sophokles73 reviewed Apr 3, 2024

View reviewed changes

Moved uuid_n_ms_in_past function logic to within pub(crate) fn on UUI…

ea89473

…DBuilder to allow other tests access

PLeVasseur force-pushed the feature/uuidbuilder_thread-safe_proposal branch from aa99f08 to ea89473 Compare April 3, 2024 11:15

sophokles73 approved these changes Apr 3, 2024

View reviewed changes

sophokles73 merged commit 1ee4f8d into eclipse-uprotocol:main Apr 3, 2024
10 checks passed

PLeVasseur deleted the feature/uuidbuilder_thread-safe_proposal branch April 3, 2024 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guarantee uE implementors always get the correct UUID when calling `UUIDBuilder::build()` by using internally held singleton #74

Guarantee uE implementors always get the correct UUID when calling `UUIDBuilder::build()` by using internally held singleton #74

PLeVasseur commented Mar 29, 2024

evshary left a comment

evshary Mar 30, 2024

evshary Mar 30, 2024

AnotherDaniel left a comment

sophokles73 Apr 2, 2024

PLeVasseur Apr 2, 2024

PLeVasseur Apr 2, 2024

sophokles73 Apr 3, 2024

PLeVasseur Apr 3, 2024

sophokles73 Apr 3, 2024

PLeVasseur Apr 3, 2024

PLeVasseur Apr 3, 2024

sophokles73 Apr 3, 2024

sophokles73 left a comment

		thread::sleep(std::time::Duration::from_millis(created_millis_ago));

		pub(crate) const BITMASK_CLEAR_VERSION: u64 = 0xffff_ffff_ffff_0fff;
		const BITMASK_CLEAR_VARIANT: u64 = 0x3fff_ffff_ffff_ffff;

Guarantee uE implementors always get the correct UUID when calling UUIDBuilder::build() by using internally held singleton #74

Guarantee uE implementors always get the correct UUID when calling UUIDBuilder::build() by using internally held singleton #74

Conversation

PLeVasseur commented Mar 29, 2024

What

Why

How

Mutex vs Atomic Compare-and-Swap (CAS) Performance

evshary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnotherDaniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophokles73 left a comment

Choose a reason for hiding this comment

Guarantee uE implementors always get the correct UUID when calling `UUIDBuilder::build()` by using internally held singleton #74

Guarantee uE implementors always get the correct UUID when calling `UUIDBuilder::build()` by using internally held singleton #74