Skip to content

Expose UUIDv7 factory statistics#77

Merged
dexcompiler merged 4 commits into
mainfrom
codex/70-uuidv7-statistics
May 15, 2026
Merged

Expose UUIDv7 factory statistics#77
dexcompiler merged 4 commits into
mainfrom
codex/70-uuidv7-statistics

Conversation

@dexcompiler
Copy link
Copy Markdown
Owner

Summary

  • add opt-in UuidV7FactoryStatistics snapshot counters for generation, rollback, overflow, spin-wait, logical drift, CAS retries, and random-buffer refills
  • wire statistics through UuidV7Factory and DI helpers without changing the disabled hot path beyond nullable checks
  • add deterministic C# coverage, F# property coverage, docs, and demo benchmark output

Validation

  • dotnet build Clockworks.sln -v minimal
  • dotnet test Clockworks.sln -v minimal -m:1
  • DOCS_VERSION=dev npm run docs:build (from docs/)
  • dotnet run --project demo/Clockworks.Demo -- uuidv7 --bench
  • dotnet pack src/Clockworks.csproj -c Release -v minimal

Benchmark sample from this machine:

  • disabled: 2,376,360 ops/sec (420.8 ns/op)
  • statistics enabled: 2,263,996 ops/sec (441.7 ns/op, 5.0% vs disabled)

Fixes #70

@dexcompiler
Copy link
Copy Markdown
Owner Author

@copilot review this pr

Copy link
Copy Markdown
Contributor

Copilot AI commented May 15, 2026

@copilot review this pr

Code Review Summary

Build: ✅ clean (0 warnings) · Tests: ✅ 131/131 passed


Design & correctness

The overall approach is solid: opt-in via nullable UuidV7FactoryStatistics?, atomic updates via Interlocked, and a CAS-based InterlockedMax for MaxLogicalDriftMs. The _statistics is null fast-exit in RecordAllocationDecision keeps the disabled path to a single null check per allocation.

A few things worth noting:

  1. Counter slightly over-counts under CAS contention. In both AllocateTimestampAndCounter and FillGuids, RecordCounterOverflow() (and RecordSpinWait()) is called before the CAS succeeds. If the CAS loses and the loop retries, the overflow condition might be re-evaluated and double-counted. In practice this is rare and acceptable for diagnostics counters, but it's worth a comment so future readers don't assume exact accuracy under heavy contention.

  2. Reset() and Snapshot() are not atomic across all fields. This is standard for multi-field lock-free statistics and the docs describe it as a point-in-time capture, so it's fine — just worth acknowledging in the XML doc for Reset() (e.g. "individual counter resets are not atomic with respect to each other").

  3. DI overload with custom TimeProvider passes an instance to AddSingleton. services.AddSingleton<IUuidV7Factory>(new UuidV7Factory(...)) means the DI container won't call Dispose() on the factory when the container is disposed. This matches the pre-existing AddLockFreeGuidFactory(TimeProvider, ...) overload so it's consistent, but both overloads silently transfer ownership of the UuidV7Factory to the caller. A note in the XML doc would help.

  4. _overflowBehavior field is stored but only _effectiveOverflowBehavior drives behavior. This was present before the PR — no action needed, just noting it's not new.

Tests

The test suite is comprehensive:

  • All 8 counters have targeted deterministic tests.
  • BarrierTimeProvider is a clever way to maximize CAS contention reproducibly.
  • The NewGuid_RecordsSpinWait_... test's SpinWait.SpinUntil + 2-second timeout window is reasonable; the cleanup finally block prevents hangs if Assert.True(observedSpin) fires.
  • Two new FsCheck properties (GeneratedCount, MaxLogicalDrift) provide good cross-validation.

Minor nits (non-blocking)

  • UuidV7FactoryStatisticsSnapshot is a readonly record struct with 8 positional parameters — callers using positional syntax will break if the parameter order ever changes. Named-parameter usage in Snapshot() would guard against that.
  • The F# DeterministicRandomNumberGenerator type is redefined locally in UuidV7FactoryProperties.fs rather than shared from a common helper. Not wrong, but creates drift risk if the C# version changes.

No security issues found. The Interlocked usage is correct throughout.

@dexcompiler
Copy link
Copy Markdown
Owner Author

Verification follow-up after Copilot review:

  • Verified the overflow/spin-wait observation. The current counters are intentionally diagnostic path-entry/wait-attempt counters rather than a globally serialized event log. I kept the hot path as-is and documented that explicitly in XML docs and user docs, avoiding extra synchronization or post-CAS bookkeeping.
  • Verified the multi-counter Snapshot()/Reset() point. I added XML remarks and guide text clarifying that each field is atomic, but the aggregate operation is not linearizable across all counters.
  • Verified the custom TimeProvider DI ownership issue and fixed it in scope: both custom-time AddLockFreeGuidFactory overloads now register factory delegates, so the container disposes the created UuidV7Factory. Externally supplied TimeProvider/RNG instances remain caller-owned. Added a disposal regression test.
  • Switched Snapshot() construction to named arguments for readability/parameter-order resilience.

I treated the stored _overflowBehavior field and duplicated F# deterministic RNG helper as non-blocking adjacent notes: neither changes behavior or the #70 mitigation surface, and the helper duplication predates this PR's statistics contract.

Validation after the follow-up commit:

  • dotnet test tests/Clockworks.Tests.csproj --filter UuidV7FactoryStatisticsTests -v minimal
  • dotnet test Clockworks.sln -v minimal -m:1
  • DOCS_VERSION=dev npm run docs:build
  • dotnet pack src/Clockworks.csproj -c Release -v minimal
  • Refreshed GitHub checks are green.

@dexcompiler dexcompiler merged commit 7f1e7ac into main May 15, 2026
6 checks passed
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.

Feature: expose UuidV7Factory skew, overflow, and contention statistics

2 participants