V1 Generator Should not Always Increment Sequence Counter

[uuid-rs]

V1 Generator Should not Always Increment Sequence Counter

[radix]

I will try to work on this today.

[radix]

Hi, if you want to create a PR, I can help review it! (I have no authority in the uuid project, just offering my assistance)

[radix]

Hi @posborne, do you still have any time to implement this change? If not, I can take a crack at it.

[kinggoesgaming]

hi @posborne are you still interested in providing a PR for this? I want this to be part of 0.7 release. Else @radix can work on it.

[radix]

Any recommendations for how to handle storing the "last updated" time in the context?

Stable rust only exports AtomicUsize, not things like AtomicU64 and AtomicU32. And even if those types were stabilized, they would not all be available on all platforms: https://doc.rust-lang.org/src/core/sync/atomic.rs.html#1439

If we want the default UuidV1Context datastructure to continue to support multi-threaded use, then we must use some internally-mutable type that supports multithreaded access. This means either Atomics or Mutexes...

maybe there's some way we could use Atomics, but there are apparently even platforms where usize == u16. That means we would need a way to store 96 bits of timestamp across up to 6 AtomicUsize values...

Just using Arc<Mutex<Timestamp>> for the timestamp would work, but this could end up slowing down new_v1 by a lot.

[radix]

I have a WIP in the linked PR #136, using the Mutex approach for keeping track of the previous time.

Surprisingly to me, this only increases the runtime of v1 generation to about 2x the previous time. But it has another drawback too: the default context now requires std whereas the old one would work without it (mutexes are only available with std). Is this really okay? Should we add another context implementation identical to the old one, so we have something for people who want to generate v1 uuids without std?

I'm also wondering what the drawbacks to the old context. The RFC does say that we should update it when the timestamp might have stayed the same or gone backwards, but it doesn't forbid or even seem to say we SHOULDN'T increment it when the timestamp moves forward.

Of course, the trait is definitely a good idea, I'm just wondering what problems we're going to solve with the new implementation. Does anyone know if there was any rationale for this discussed elsewhere (during the libs blitz perhaps)?

[radix]

I put up a more conservative PR at #137. It only adds the trait, and doesn't change the way sequences are generated. It's all backwards compatible.

[kinggoesgaming]

Just a note that we don’t merge that PR until 0.6 is released

[Dylan-DPC-zz]

@posborne the trait merged in #137 and released in 0.6.0 fullfills the needs of this issue. If anything else is needed, you may create a new issue for it :)

[PrismaPhonic]

I know this has already been closed but I think offering a trait to "solve" this problem is the wrong approach. The default use of ClockSequence here is just plain wrong and should be corrected to follow spec.

[Dylan-DPC-zz]

@PrismaPhonic thanks. We are always open to discussing issues and decisions. Moving this to a new issue for better traction.