Roaring Bitmap: address PR review feedback

Copilot · Copilot · commit 65ac9f1d4a24 · 2026-04-27T12:11:49.000-07:00
1. RoaringBitmapDataTests: cast actual0 to int when indexing bool[];

   add an explicit non-negative guard for clarity.

2. RoaringBitmap.ByteSize: docstring now states the per-entry

   SortedDictionary node overhead is included (the implementation

   already accounted for it).

3. RSetBit.NeedInitialUpdate: validate offset/value against a copy

   of the input before the framework allocates an empty object;

   write the error and return false on bad input so malformed

   R.SETBIT no longer leaves an empty key behind.

4. RoaringBitmapObject default ctor: align Size with the deserialized

   ctor by setting Size = ObjectOverhead + bitmap.ByteSize so

   freshly-created and round-tripped objects report identical memory

   baselines and later ByteSize-based deltas don't double-count.

5. (Reviewer suggested removing 'using Garnet.common;' as unused;

   that namespace owns RespMemoryWriter and is required, so the

   using stays.)

All 27 data-structure + 14 RESP integration tests still pass.
diff --git a/main/GarnetServer/Extensions/RoaringBitmap/RoaringBitmap.cs b/main/GarnetServer/Extensions/RoaringBitmap/RoaringBitmap.cs
@@ -178,15 +178,20 @@ public long BitPos(int bit, uint from = 0)
             }
         }
 
-        /// <summary>Estimated heap byte cost. Excludes the .NET object header overhead and the SortedDictionary node overhead.</summary>
+        /// <summary>
+        /// Estimated heap byte cost, including an approximate base-object cost and
+        /// approximate per-entry <see cref="SortedDictionary{TKey, TValue}"/> node
+        /// overhead. Excludes the .NET object header overhead.
+        /// </summary>
         public long ByteSize
         {
             get
             {
-                long sum = 24; // base object overhead estimate
+                long sum = 24; // approximate RoaringBitmap instance/base-object cost
                 foreach (var kv in chunks)
                 {
-                    // Per-entry: key (2B), reference (8B), red-black tree node overhead (~40B), and container.
+                    // Per-entry estimate: key (2B), reference (8B), red-black tree
+                    // node overhead (~40B), and the container's own footprint.
                     sum += 50 + kv.Value.ByteSize;
                 }
                 return sum;
diff --git a/main/GarnetServer/Extensions/RoaringBitmap/RoaringBitmapCommands.cs b/main/GarnetServer/Extensions/RoaringBitmap/RoaringBitmapCommands.cs
@@ -47,7 +47,28 @@ public sealed class RSetBit : CustomObjectFunctions
         private static ReadOnlySpan<byte> ErrOffset => "ERR bit offset is not an unsigned 32-bit integer"u8;
         private static ReadOnlySpan<byte> ErrValue => "ERR bit value must be 0 or 1"u8;
 
-        public override bool NeedInitialUpdate(ReadOnlyMemory<byte> key, ref ObjectInput input, ref RespMemoryWriter writer) => true;
+        public override bool NeedInitialUpdate(ReadOnlyMemory<byte> key, ref ObjectInput input, ref RespMemoryWriter writer)
+        {
+            // Validate args BEFORE the framework allocates the empty object so that
+            // a malformed R.SETBIT does not leave a tombstone-style empty key behind.
+            // Walk a copy of the input so Updater can re-parse from offset 0.
+            var validation = input;
+            int offset = 0;
+            var offsetArg = GetNextArg(ref validation, ref offset);
+            var bitArg = GetNextArg(ref validation, ref offset);
+
+            if (!RoaringBitmapArgs.TryParseUInt32(offsetArg, out _))
+            {
+                writer.WriteError(ErrOffset);
+                return false;
+            }
+            if (!RoaringBitmapArgs.TryParseBit(bitArg, out _))
+            {
+                writer.WriteError(ErrValue);
+                return false;
+            }
+            return true;
+        }
 
         public override bool Updater(ReadOnlyMemory<byte> key, ref ObjectInput input, IGarnetObject value, ref RespMemoryWriter writer, ref RMWInfo rmwInfo)
         {
diff --git a/main/GarnetServer/Extensions/RoaringBitmap/RoaringBitmapObject.cs b/main/GarnetServer/Extensions/RoaringBitmap/RoaringBitmapObject.cs
@@ -27,6 +27,10 @@ public RoaringBitmapObject(byte type)
             : base(type, expiration: 0, size: ObjectOverhead)
         {
             bitmap = new RoaringBitmap();
+            // Keep size accounting consistent with the deserialized constructor and
+            // with later mutation deltas based on bitmap.ByteSize so the empty-bitmap
+            // baseline is counted exactly once.
+            this.Size = ObjectOverhead + bitmap.ByteSize;
         }
 
         public RoaringBitmapObject(byte type, BinaryReader reader)
diff --git a/test/Garnet.test/RoaringBitmapDataTests.cs b/test/Garnet.test/RoaringBitmapDataTests.cs
@@ -172,7 +172,7 @@ public void SmallUniverse_BitPos_MatchesOracle()
                 // uint32 universe — past the set range any value qualifies, so
                 // accept any value >= from that's not in the bits array.
                 ClassicAssert.GreaterOrEqual(actual0, from);
-                ClassicAssert.IsTrue(actual0 == 0 || (actual0 < bits.LongLength ? !bits[actual0] : true));
+                ClassicAssert.IsTrue(actual0 == 0 || (actual0 >= 0 && actual0 < bits.LongLength ? !bits[(int)actual0] : true));
             }
         }
 

Original file line number	Diff line number	Diff line change
`@@ -178,15 +178,20 @@ public long BitPos(int bit, uint from = 0)`
`178`	`178`	`}`
`179`	`179`	`}`
`180`	`180`
`181`		`- /// <summary>Estimated heap byte cost. Excludes the .NET object header overhead and the SortedDictionary node overhead.</summary>`
	`181`	`+ /// <summary>`
	`182`	`+ /// Estimated heap byte cost, including an approximate base-object cost and`
	`183`	`+ /// approximate per-entry <see cref="SortedDictionary{TKey, TValue}"/> node`
	`184`	`+ /// overhead. Excludes the .NET object header overhead.`
	`185`	`+ /// </summary>`
`182`	`186`	`public long ByteSize`
`183`	`187`	`{`
`184`	`188`	`get`
`185`	`189`	`{`
`186`		`- long sum = 24; // base object overhead estimate`
	`190`	`+ long sum = 24; // approximate RoaringBitmap instance/base-object cost`
`187`	`191`	`foreach (var kv in chunks)`
`188`	`192`	`{`
`189`		`- // Per-entry: key (2B), reference (8B), red-black tree node overhead (~40B), and container.`
	`193`	`+ // Per-entry estimate: key (2B), reference (8B), red-black tree`
	`194`	`+ // node overhead (~40B), and the container's own footprint.`
`190`	`195`	`sum += 50 + kv.Value.ByteSize;`
`191`	`196`	`}`
`192`	`197`	`return sum;`
Original file line number	Diff line number	Diff line change
`@@ -27,6 +27,10 @@ public RoaringBitmapObject(byte type)`
`27`	`27`	`: base(type, expiration: 0, size: ObjectOverhead)`
`28`	`28`	`{`
`29`	`29`	`bitmap = new RoaringBitmap();`
	`30`	`+ // Keep size accounting consistent with the deserialized constructor and`
	`31`	`+ // with later mutation deltas based on bitmap.ByteSize so the empty-bitmap`
	`32`	`+ // baseline is counted exactly once.`
	`33`	`+ this.Size = ObjectOverhead + bitmap.ByteSize;`
`30`	`34`	`}`
`31`	`35`
`32`	`36`	`public RoaringBitmapObject(byte type, BinaryReader reader)`
Original file line number	Diff line number	Diff line change
`@@ -172,7 +172,7 @@ public void SmallUniverse_BitPos_MatchesOracle()`
`172`	`172`	`// uint32 universe — past the set range any value qualifies, so`
`173`	`173`	`// accept any value >= from that's not in the bits array.`
`174`	`174`	`ClassicAssert.GreaterOrEqual(actual0, from);`
`175`		`- ClassicAssert.IsTrue(actual0 == 0 \|\| (actual0 < bits.LongLength ? !bits[actual0] : true));`
	`175`	`+ ClassicAssert.IsTrue(actual0 == 0 \|\| (actual0 >= 0 && actual0 < bits.LongLength ? !bits[(int)actual0] : true));`
`176`	`176`	`}`
`177`	`177`	`}`
`178`	`178`