Restore counter-based CAS semantics in segcache#23
Closed
thinkingfish wants to merge 1 commit into
Closed
Conversation
The hashtable rewrite (#10) replaced the per-bucket CAS counter with the item's packed location bits as the CAS token. This regressed memcache CAS semantics for downstream consumers: tokens were no longer small, deterministic, monotonically increasing integers, and they changed whenever an item was relocated. The memcache 'cas stored' integration test fails with EXISTS where STORED is expected. Restore the original semantics with a per-bucket atomic CAS counter, indexed by a key's primary (first-choice) bucket: - incremented on every successful hashtable insert - incremented again on a successful CAS check, matching the original double-bump behavior of try_update_cas + insert - left untouched by relocation (cas_location), removal, and ghost conversion - reset by clear() Item::cas() and the cas() operation now read this counter instead of the location bits. Add unit tests covering the counter lifecycle and an end-to-end test pinning the exact token sequence the original implementation produced. https://claude.ai/code/session_01B7MKPvEtTUmRX996r1uKku
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The hashtable rewrite (#10) replaced the per-bucket CAS counter with the item's packed location bits as the CAS token. This regressed memcache CAS semantics for downstream consumers: the
cas storedintegration test receivesEXISTSwhereSTOREDis expected, which is why the downstream segcache git dependency has stayed pinned at 5d1d870.Two concrete problems with location-derived tokens:
Fix
Restore the original semantics with a per-bucket
AtomicU32CAS counter inMultiChoiceHashtable, indexed by a key's primary (first-choice) bucket — the new symmetric-slot bucket design has no metadata word to hold it, so the counters live in a parallel array (4 bytes per 64-byte bucket):try_update_cas+insertcas_location), removal, and ghost conversionclear()Item::cas()andSegcache::cas()now read this counter instead of the location bits. Thecas()lookup also counts as a hit (frequency update) again, as it did in the original implementation.Testing
test_cas_value_counterunit test covers the counter lifecycle at the hashtable level (insert bumps, relocation/removal don't, clear resets).cas_deterministicend-to-end test pins the exact token sequence the original implementation produced (1 after first set, 2 after overwrite, 4 after a successful CAS).cargo test --workspace,cargo test -p segcache --features loom -- loom(CI invocation, 8/8 models pass), andcargo clippy --all-targetsare all green.After this lands, the downstream dependency bump past c2f43b8 should no longer regress the
cas storedtest.https://claude.ai/code/session_01B7MKPvEtTUmRX996r1uKku
Generated by Claude Code