Single-probe TryAdd across all four collections

Previously TryAdd (and therefore Add) walked the probe chain twice on
every call: once via ContainsKey/Contains, then again via the indexer
setter / InsertNonZero / InsertNonDefault. Replace both walks with a
single ProbeForInsert-style walk that either lands on the existing
entry (return false) or on the first empty slot (insert in place).

Affects IntDictionary, CelerityDictionary, IntSet, and CeleritySet.
Behaviour is identical -- duplicate-key Add still throws, TryAdd still
returns false on duplicates and leaves the existing value untouched --
but bulk-loads via the new IEnumerable<KeyValuePair> constructor and
any Add-heavy hot path now do half the probe work.

Pinned by TryAddProbeCountTests, which uses a counting IHashProvider
to assert that TryAdd calls Hash exactly once on both the new-key and
duplicate-key paths across all four collections.

Closes issue #23.

Author: marius-bughiu

CHANGELOG.md

@@ -53,6 +53,7 @@ All notable changes to Celerity are documented here. This project follows [Keep
 
 ### Changed
 
+- `TryAdd` (and therefore `Add`) on `IntDictionary<TValue, THasher>`, `CelerityDictionary<TKey, TValue, THasher>`, `IntSet<THasher>`, and `CeleritySet<T, THasher>` now walks the probe chain exactly **once** per call instead of twice. The previous implementation called `ContainsKey` / `Contains` followed by the indexer setter / `InsertNon*` helper, each starting its own probe walk; the rewrite uses a single `ProbeForInsert`-style walk that either lands on the existing entry (return `false`) or on the first empty slot (insert in place). Behaviour is identical to before — including the duplicate-key contract on `Add` and the "unchanged on duplicate" contract on `TryAdd` — but bulk-loads via the new `IEnumerable<KeyValuePair<,>>` constructor and any `Add`-heavy hot path now do roughly half the probe work. Closes issue #23. Pinned by `TryAddProbeCountTests`, which uses a counting `IHashProvider` to assert that `TryAdd` calls `Hash` exactly once on both the new-key and duplicate-key paths across all four collections.
 - The `IntDictionary` `EMPTY_VALUE` field is now `static readonly` instead of an instance field. No behavior change; just removes per-instance overhead.
 
 ## [0.1.0] - initial releases

ISSUES.md

@@ -222,3 +222,59 @@ Both constructors now throw `ArgumentOutOfRangeException` for `capacity < 0`, `l
 - **#18** — Robin Hood probing experiment (0.6.0).
 - **#19** — SIMD-accelerated probing experiment (0.6.0).
 - **#20** — Struct-of-arrays layout experiment (0.6.0).
+
+---
+
+## #23 — `TryAdd` / `Add` walk the probe chain twice on every insert
+
+- **type**: perf / code-quality
+- **severity**: medium
+- **milestone**: 1.1.0
+- **status**: fixed in 1.1.0
+
+### Description
+
+`TryAdd(key, value)` (on both dictionaries and both sets) is currently implemented as:
+
+```csharp
+public bool TryAdd(int key, TValue value)
+{
+    // ... handle out-of-band zero/default key ...
+    if (ContainsKey(key))   // probe 1 — full ProbeForKey walk
+        return false;
+    this[key] = value;      // probe 2 — full ProbeForInsert walk inside the indexer setter
+    return true;
+}
+```
+
+That's two independent walks of the probe chain on the *common* path (the new-key path) when one would suffice. The same shape applies to `IntSet.TryAdd` / `CeleritySet.TryAdd` (both call `Contains` then `InsertNonZero` / `InsertNonDefault`).
+
+The double probe matters in three places:
+
+1. `Add(key, value)` — delegates to `TryAdd`, so every duplicate-throwing insert pays the same tax.
+2. The new `IEnumerable<KeyValuePair<TKey, TValue>>` constructor (added in 1.1.0) — bulk-loads via `Add`, so loading an N-element source does ~2N probe-chain walks instead of N. This is a regression on the headline 1.1.0 ergonomic API.
+3. Any caller doing `if (!dict.TryAdd(...)) ...` on a hot path.
+
+### Fix
+
+A single `ProbeForInsert(key)` call already returns either:
+
+- the index of an existing entry (`_keys[index] == key`), or
+- the first empty slot in the probe chain (`_keys[index] == EMPTY_KEY` / `default(TKey)`).
+
+`TryAdd` only needs to inspect that slot. If it's not empty, the key already exists → return false. Otherwise insert at that slot directly:
+
+```csharp
+if (_count >= _threshold) Resize();
+int index = ProbeForInsert(key);
+if (_keys[index] != EMPTY_KEY) return false;  // already present
+_keys[index] = key;
+_values[index] = value;
+_count++;
+_version++;
+return true;
+```
+
+`Add(key, value)` is left as a thin throwing wrapper around `TryAdd`. Behaviour is identical to before; the only change is the elimination of the redundant probe walk.
+
+A regression test using a probe-counting `IHashProvider` should accompany the fix — it pins the contract "TryAdd does exactly one probe walk on the new-key path" so a future refactor can't quietly re-introduce the doubling.

src/Celerity.Tests/Collections/TryAddProbeCountTests.cs

@@ -0,0 +1,215 @@
+using Celerity.Collections;
+using Celerity.Hashing;
+
+namespace Celerity.Tests.Collections;
+
+/// <summary>
+/// Regression tests for issue #23. <see cref="IntDictionary{TValue, THasher}.TryAdd"/>,
+/// <see cref="CelerityDictionary{TKey, TValue, THasher}.TryAdd"/>,
+/// <see cref="IntSet{THasher}.TryAdd"/>, and
+/// <see cref="CeleritySet{T, THasher}.TryAdd"/> historically walked the probe chain
+/// twice on the new-key path: once via <c>ContainsKey</c>/<c>Contains</c>, then again
+/// via the indexer setter / <c>InsertNon*</c> helper. The fix collapses both walks
+/// into a single <c>ProbeForInsert</c>. These tests pin that contract by counting how
+/// many times the underlying <see cref="IHashProvider{T}"/>'s <c>Hash</c> method is
+/// called: each probe-chain walk starts with exactly one <c>Hash</c> call (the
+/// subsequent linear-probe steps don't re-hash), so hash-call count is a faithful
+/// proxy for probe-chain walk count.
+///
+/// All tests pre-size the collection so no <c>Resize</c> happens during the asserted
+/// region — <c>Resize</c> would otherwise add hash calls of its own as it re-inserts
+/// the existing entries into the new array.
+/// </summary>
+public class TryAddProbeCountTests
+{
+    private static int _hashCallCount;
+
+    /// <summary>
+    /// A counting hasher for <see cref="int"/> keys. The implementation is a
+    /// minimal Wang-style mix so distinct keys distribute across the table; the
+    /// only test-relevant aspect is that each call increments a static counter.
+    /// </summary>
+    private struct CountingIntHasher : IHashProvider<int>
+    {
+        public int Hash(int key)
+        {
+            _hashCallCount++;
+            unchecked
+            {
+                uint x = (uint)key;
+                x = ((x >> 16) ^ x) * 0x45d9f3b;
+                x = ((x >> 16) ^ x) * 0x45d9f3b;
+                x = (x >> 16) ^ x;
+                return (int)x;
+            }
+        }
+    }
+
+    /// <summary>
+    /// A counting hasher for <see cref="string"/> keys.
+    /// </summary>
+    private struct CountingStringHasher : IHashProvider<string>
+    {
+        public int Hash(string key)
+        {
+            _hashCallCount++;
+            // Deliberately use a stable, deterministic mix; we don't care about
+            // distribution for these tests, only that the call is counted.
+            return key.GetHashCode();
+        }
+    }
+
+    [Fact]
+    public void IntDictionary_TryAdd_NewKey_DoesExactlyOneProbeWalk()
+    {
+        // Pre-size so the asserted inserts never resize.
+        var map = new IntDictionary<int, CountingIntHasher>(capacity: 64);
+        _hashCallCount = 0;
+
+        // 10 brand-new (non-zero) keys. Pre-fix this allocated 20 hash calls.
+        for (int i = 1; i <= 10; i++)
+            Assert.True(map.TryAdd(i, i * 10));
+
+        Assert.Equal(10, _hashCallCount);
+        Assert.Equal(10, map.Count);
+    }
+
+    [Fact]
+    public void IntDictionary_TryAdd_DuplicateKey_DoesExactlyOneProbeWalk()
+    {
+        var map = new IntDictionary<int, CountingIntHasher>(capacity: 64);
+        for (int i = 1; i <= 5; i++)
+            map.TryAdd(i, i);
+
+        _hashCallCount = 0;
+        for (int i = 1; i <= 5; i++)
+            Assert.False(map.TryAdd(i, -1));
+
+        Assert.Equal(5, _hashCallCount);
+        // Original values must remain untouched on the duplicate path.
+        for (int i = 1; i <= 5; i++)
+            Assert.Equal(i, map[i]);
+    }
+
+    [Fact]
+    public void IntDictionary_Add_NewKey_DoesExactlyOneProbeWalk()
+    {
+        var map = new IntDictionary<int, CountingIntHasher>(capacity: 64);
+        _hashCallCount = 0;
+
+        for (int i = 1; i <= 10; i++)
+            map.Add(i, i);
+
+        Assert.Equal(10, _hashCallCount);
+        Assert.Equal(10, map.Count);
+    }
+
+    [Fact]
+    public void CelerityDictionary_TryAdd_NewKey_DoesExactlyOneProbeWalk()
+    {
+        var map = new CelerityDictionary<string, int, CountingStringHasher>(capacity: 64);
+        _hashCallCount = 0;
+
+        for (int i = 1; i <= 10; i++)
+            Assert.True(map.TryAdd($"k{i}", i));
+
+        Assert.Equal(10, _hashCallCount);
+        Assert.Equal(10, map.Count);
+    }
+
+    [Fact]
+    public void CelerityDictionary_TryAdd_DuplicateKey_DoesExactlyOneProbeWalk()
+    {
+        var map = new CelerityDictionary<string, int, CountingStringHasher>(capacity: 64);
+        for (int i = 1; i <= 5; i++)
+            map.TryAdd($"k{i}", i);
+
+        _hashCallCount = 0;
+        for (int i = 1; i <= 5; i++)
+            Assert.False(map.TryAdd($"k{i}", -1));
+
+        Assert.Equal(5, _hashCallCount);
+        for (int i = 1; i <= 5; i++)
+            Assert.Equal(i, map[$"k{i}"]);
+    }
+
+    [Fact]
+    public void IntSet_TryAdd_NewItem_DoesExactlyOneProbeWalk()
+    {
+        var set = new IntSet<CountingIntHasher>(capacity: 64);
+        _hashCallCount = 0;
+
+        for (int i = 1; i <= 10; i++)
+            Assert.True(set.TryAdd(i));
+
+        Assert.Equal(10, _hashCallCount);
+        Assert.Equal(10, set.Count);
+    }
+
+    [Fact]
+    public void IntSet_TryAdd_DuplicateItem_DoesExactlyOneProbeWalk()
+    {
+        var set = new IntSet<CountingIntHasher>(capacity: 64);
+        for (int i = 1; i <= 5; i++)
+            set.TryAdd(i);
+
+        _hashCallCount = 0;
+        for (int i = 1; i <= 5; i++)
+            Assert.False(set.TryAdd(i));
+
+        Assert.Equal(5, _hashCallCount);
+        Assert.Equal(5, set.Count);
+    }
+
+    [Fact]
+    public void CeleritySet_TryAdd_NewItem_DoesExactlyOneProbeWalk()
+    {
+        var set = new CeleritySet<string, CountingStringHasher>(capacity: 64);
+        _hashCallCount = 0;
+
+        for (int i = 1; i <= 10; i++)
+            Assert.True(set.TryAdd($"v{i}"));
+
+        Assert.Equal(10, _hashCallCount);
+        Assert.Equal(10, set.Count);
+    }
+
+    [Fact]
+    public void CeleritySet_TryAdd_DuplicateItem_DoesExactlyOneProbeWalk()
+    {
+        var set = new CeleritySet<string, CountingStringHasher>(capacity: 64);
+        for (int i = 1; i <= 5; i++)
+            set.TryAdd($"v{i}");
+
+        _hashCallCount = 0;
+        for (int i = 1; i <= 5; i++)
+            Assert.False(set.TryAdd($"v{i}"));
+
+        Assert.Equal(5, _hashCallCount);
+        Assert.Equal(5, set.Count);
+    }
+
+    [Fact]
+    public void IntDictionary_TryAdd_PreservesExistingValueOnDuplicate()
+    {
+        // Behavioural guard: TryAdd must not overwrite the existing value when
+        // it returns false. The single-probe rewrite has to be careful here
+        // because ProbeForInsert returns the existing slot — we must read,
+        // detect, and bail out before writing.
+        var map = new IntDictionary<int, CountingIntHasher>(capacity: 64);
+        map.TryAdd(7, 700);
+
+        Assert.False(map.TryAdd(7, -1));
+        Assert.Equal(700, map[7]);
+    }
+
+    [Fact]
+    public void CelerityDictionary_TryAdd_PreservesExistingValueOnDuplicate()
+    {
+        var map = new CelerityDictionary<string, int, CountingStringHasher>(capacity: 64);
+        map.TryAdd("k7", 700);
+
+        Assert.False(map.TryAdd("k7", -1));
+        Assert.Equal(700, map["k7"]);
+    }
+}

src/Celerity/Collections/CelerityDictionary.cs

@@ -290,10 +290,22 @@ public bool TryAdd(TKey key, TValue value)
             return true;
         }
 
-        if (ContainsKey(key))
+        // Single probe: ProbeForInsert returns either the slot of an existing
+        // entry or the first empty slot in the chain. If it's the former, the
+        // key already exists; otherwise we insert here directly. This avoids
+        // the double probe-chain walk that `if (ContainsKey(key)) ...; this[key] = value;`
+        // would do.
+        if (_count >= _threshold)
+            Resize();
+
+        int index = ProbeForInsert(key);
+        if (!EqualityComparer<TKey>.Default.Equals(_keys[index], default(TKey)))
             return false;
 
-        this[key] = value;
+        _keys[index] = key;
+        _values[index] = value;
+        _count++;
+        _version++;
         return true;
     }
 

src/Celerity/Collections/CeleritySet.cs

@@ -100,10 +100,24 @@ public bool TryAdd(T item)
             return true;
         }
 
-        if (Contains(item))
-            return false;
+        // Single probe: walk the probe chain once and either spot the existing
+        // entry (return false) or land on an empty slot and insert in place.
+        // Avoids the double walk of `if (Contains(item)) ...; InsertNonDefault(item);`.
+        if (_count >= _threshold)
+            Resize();
+
+        int size = _slots.Length;
+        int index = _hasher.Hash(item) & (size - 1);
+
+        while (!EqualityComparer<T>.Default.Equals(_slots[index], default(T)))
+        {
+            if (EqualityComparer<T>.Default.Equals(_slots[index], item))
+                return false;
+            index = (index + 1) & (size - 1);
+        }
 
-        InsertNonDefault(item);
+        _slots[index] = item;
+        _count++;
         return true;
     }
 

src/Celerity/Collections/IntDictionary.cs

@@ -345,10 +345,22 @@ public bool TryAdd(int key, TValue value)
             return true;
         }
 
-        if (ContainsKey(key))
+        // Single probe: ProbeForInsert returns either the slot of an existing
+        // entry or the first empty slot in the chain. If it's the former, the
+        // key already exists; otherwise we insert here directly. This avoids
+        // the double probe-chain walk that `if (ContainsKey(key)) ...; this[key] = value;`
+        // would do.
+        if (_count >= _threshold)
+            Resize();
+
+        int index = ProbeForInsert(key);
+        if (_keys[index] != EMPTY_KEY)
             return false;
 
-        this[key] = value;
+        _keys[index] = key;
+        _values[index] = value;
+        _count++;
+        _version++;
         return true;
     }
 

src/Celerity/Collections/IntSet.cs

@@ -124,10 +124,24 @@ public bool TryAdd(int item)
             return true;
         }
 
-        if (Contains(item))
-            return false;
+        // Single probe: walk the probe chain once and either spot the existing
+        // entry (return false) or land on an empty slot and insert in place.
+        // Avoids the double walk of `if (Contains(item)) ...; InsertNonZero(item);`.
+        if (_count >= _threshold)
+            Resize();
+
+        int size = _slots.Length;
+        int index = _hasher.Hash(item) & (size - 1);
+
+        while (_slots[index] != EMPTY_SLOT)
+        {
+            if (_slots[index] == item)
+                return false;
+            index = (index + 1) & (size - 1);
+        }
 
-        InsertNonZero(item);
+        _slots[index] = item;
+        _count++;
         return true;
     }