Refactor BITPOS Implementation #1016

vazois · 2025-02-12T23:09:39Z

This PR fixes the BITPOS to support BIT search correctly.

prvyk · 2025-02-13T02:46:12Z

This is great; I couldn't find time for this. But one thing, there was a test which I couldn't pass last try, and this one doesn't seem to pass it either (maybe I got the test wrong). Try applying this diff manually (or via git apply). It adds a few lines to BitmapBitPosOffsetsTest test.

diff --git a/test/Garnet.test/GarnetBitmapTests.cs b/test/Garnet.test/GarnetBitmapTests.cs
index 44c94f45..55ece807 100644
--- a/test/Garnet.test/GarnetBitmapTests.cs
+++ b/test/Garnet.test/GarnetBitmapTests.cs
@@ -620,6 +620,9 @@ namespace Garnet.test
                 pos = db.StringBitPosition(key, set, startOffset, endOffset);
 
                 ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset} {endOffset}");
+
+                pos = db.StringBitPosition(key, set, startOffset * 8, endOffset * 8, StringIndexType.Bit);
+                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset * 8} {endOffset * 8} bit");
             }
 
             // check negative offsets in range
@@ -635,6 +638,9 @@ namespace Garnet.test
                 expectedPos = Bitpos(buf, startOffset, endOffset, set);
                 pos = db.StringBitPosition(key, set, startOffset, endOffset);
                 ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset} {endOffset}");
+
+                pos = db.StringBitPosition(key, set, startOffset * 8, endOffset * 8, StringIndexType.Bit);
+                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset * 8} {endOffset * 8} bit");
             }
         }

You'd expect referring to the same positions (in different syntax) lead to the same result? Yet the test fails:

Failed BitmapBitPosOffsetsTest [288 ms]
Error Message:
1 False 4032 4040 bit
Assert.That(actual, Is.EqualTo(expected))
Expected: 4037
But was: 4040

Stack Trace:
at NUnit.Framework.Legacy.ClassicAssert.AreEqual(Object expected, Object actual, String message, Object[] args)
at Garnet.test.GarnetBitmapTests.BitmapBitPosOffsetsTest() in garnet/test/Garnet.test/GarnetBitmapTests.cs:line 625

at NUnit.Framework.Legacy.ClassicAssert.AreEqual(Object expected, Object actual, String message, Object[] args)
at Garnet.test.GarnetBitmapTests.BitmapBitPosOffsetsTest() in garnet/test/Garnet.test/GarnetBitmapTests.cs:line 625

(I also tried with a randomized seed a couple of times just to be sure it always passes and couldn't clear all seeds).

libs/server/Resp/Bitmap/BitmapManagerBitPos.cs

vazois · 2025-02-13T17:44:54Z

This is great; I couldn't find time for this. But one thing, there was a test which I couldn't pass last try, and this one doesn't seem to pass it either (maybe I got the test wrong). Try applying this diff manually (or via git apply). It adds a few lines to BitmapBitPosOffsetsTest test.
diff --git a/test/Garnet.test/GarnetBitmapTests.cs b/test/Garnet.test/GarnetBitmapTests.cs
index 44c94f45..55ece807 100644
--- a/test/Garnet.test/GarnetBitmapTests.cs
+++ b/test/Garnet.test/GarnetBitmapTests.cs
@@ -620,6 +620,9 @@ namespace Garnet.test
                 pos = db.StringBitPosition(key, set, startOffset, endOffset);
 
                 ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset} {endOffset}");
+
+                pos = db.StringBitPosition(key, set, startOffset * 8, endOffset * 8, StringIndexType.Bit);
+                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset * 8} {endOffset * 8} bit");
             }
 
             // check negative offsets in range
@@ -635,6 +638,9 @@ namespace Garnet.test
                 expectedPos = Bitpos(buf, startOffset, endOffset, set);
                 pos = db.StringBitPosition(key, set, startOffset, endOffset);
                 ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset} {endOffset}");
+
+                pos = db.StringBitPosition(key, set, startOffset * 8, endOffset * 8, StringIndexType.Bit);
+                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset * 8} {endOffset * 8} bit");
             }
         }
 
You'd expect referring to the same positions (in different syntax) lead to the same result? Yet the test fails:

Failed BitmapBitPosOffsetsTest [288 ms]
Error Message:
1 False 4032 4040 bit
Assert.That(actual, Is.EqualTo(expected))
Expected: 4037
But was: 4040
Stack Trace:
at NUnit.Framework.Legacy.ClassicAssert.AreEqual(Object expected, Object actual, String message, Object[] args)
at Garnet.test.GarnetBitmapTests.BitmapBitPosOffsetsTest() in garnet/test/Garnet.test/GarnetBitmapTests.cs:line 625

at NUnit.Framework.Legacy.ClassicAssert.AreEqual(Object expected, Object actual, String message, Object[] args)
at Garnet.test.GarnetBitmapTests.BitmapBitPosOffsetsTest() in garnet/test/Garnet.test/GarnetBitmapTests.cs:line 625

(I also tried with a randomized seed a couple of times just to be sure it always passes and couldn't clear all seeds).

Yeah, there were a couple of boundary conditions missing. Thanks for sharing this test. I added the fix and your unit test and it seems to pass now.

prvyk · 2025-02-13T18:05:28Z

It turns out the distribution of the default seed is somehow very specific. I tried a few, and they pass consistently except for one case: when startOffset equals endOffset. Didn't find problems when that was excluded.

diff --git a/test/Garnet.test/GarnetBitmapTests.cs b/test/Garnet.test/GarnetBitmapTests.cs
index 827f0e25..b953a005 100644
--- a/test/Garnet.test/GarnetBitmapTests.cs
+++ b/test/Garnet.test/GarnetBitmapTests.cs
@@ -607,6 +607,11 @@ namespace Garnet.test
             long expectedPos;
             long pos;
 
+	     //var r2 = new Random();
+	     //int x = r2.Next();
+	     int x = 1756844280; // 243876438 also fails with same case.
+	     r = new Random(x);
+
             for (var j = 0; j < iter; j++)
             {
                 r.NextBytes(buf);
@@ -619,12 +624,12 @@ namespace Garnet.test
                 expectedPos = Bitpos(buf, startOffset, endOffset, set);
                 pos = db.StringBitPosition(key, set, startOffset, endOffset);
 
-                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset} {endOffset}");
+                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset} {endOffset} {x}");
 
                 var startBitOffset = startOffset << 3;
                 var endBitOffset = endOffset << 3;
                 pos = db.StringBitPosition(key, set, startBitOffset, endBitOffset, StringIndexType.Bit);
-                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startBitOffset} {endBitOffset} bit");
+                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startBitOffset} {endBitOffset} bit {x}");
             }
 
             // check negative offsets in range

Result is:

Failed BitmapBitPosOffsetsTest [294 ms]
Error Message:
9 False 3264 3264 bit 1756844280
Assert.That(actual, Is.EqualTo(expected))
Expected: 3265
But was: -1

(bit output actually can differ in redis, but only when end argument isn't applied and the result is extended past end of string. This isn't the case in this test, so likely a bug.)

vazois · 2025-02-13T18:38:14Z

It turns out the distribution of the default seed is somehow very specific. I tried a few, and they pass consistently except for one case: when startOffset equals endOffset. Didn't find problems when that was excluded.

diff --git a/test/Garnet.test/GarnetBitmapTests.cs b/test/Garnet.test/GarnetBitmapTests.cs
index 827f0e25..b953a005 100644
--- a/test/Garnet.test/GarnetBitmapTests.cs
+++ b/test/Garnet.test/GarnetBitmapTests.cs
@@ -607,6 +607,11 @@ namespace Garnet.test
             long expectedPos;
             long pos;
 
+	     //var r2 = new Random();
+	     //int x = r2.Next();
+	     int x = 1756844280; // 243876438 also fails with same case.
+	     r = new Random(x);
+
             for (var j = 0; j < iter; j++)
             {
                 r.NextBytes(buf);
@@ -619,12 +624,12 @@ namespace Garnet.test
                 expectedPos = Bitpos(buf, startOffset, endOffset, set);
                 pos = db.StringBitPosition(key, set, startOffset, endOffset);
 
-                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset} {endOffset}");
+                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startOffset} {endOffset} {x}");
 
                 var startBitOffset = startOffset << 3;
                 var endBitOffset = endOffset << 3;
                 pos = db.StringBitPosition(key, set, startBitOffset, endBitOffset, StringIndexType.Bit);
-                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startBitOffset} {endBitOffset} bit");
+                ClassicAssert.AreEqual(expectedPos, pos, $"{j} {set} {startBitOffset} {endBitOffset} bit {x}");
             }
 
             // check negative offsets in range

Result is:

Failed BitmapBitPosOffsetsTest [294 ms]
Error Message:
9 False 3264 3264 bit 1756844280
Assert.That(actual, Is.EqualTo(expected))
Expected: 3265
But was: -1

(bit output actually can differ in redis, but only when end argument isn't applied and the result is extended past end of string. This isn't the case in this test, so likely a bug.)

The seed needs to be specific because the unit tests need to be deterministic.
Overall, the problem with this test is that it compares against a naive implementation of bitpos which is not ideal if that naive implementation has a bug.
In this case, the BitPos implementation that we are comparing against does not work with the BIT option only the BYTE option which explains the inconsistency for a single bit search.
You might want to try another test that sets or clears a bit at a specific position, so you know the expected pos return,
or maybe have a trace of the Redis output.
I added a simple test (a7574ea) to ensure correctness when range is a single bit.

prvyk · 2025-02-13T18:43:07Z

I'm aware of the seed requirement (it would be a bad idea to change it), I'm just using this to verify.

Now, the bitops implementation is naive, yet the exception is on the bit test. So the byte test 'passed' with the same result as the naive implementation - which suggests a BYTE range and an equivalent BIT range returned a different result. I don't think this should be happening when the end offset is specified?

Edit: Ah, but the redis BYTE range includes the entire byte when the naive bit range is limited to a bit? So it's not the same range exactly. I was wrong here, sorry.

vazois · 2025-02-13T18:58:24Z

Edit: Ah, but the redis BYTE range includes the entire byte when the naive bit range is limited to a bit? So it's not the same range exactly. I was wrong here, sorry.

Yes, I was about to explain that. Cool, if you find something else let me know. It has been tricky to test it extensively and you did a great job providing feedback.

prvyk · 2025-02-14T18:08:56Z

Edit: Ah, but the redis BYTE range includes the entire byte when the naive bit range is limited to a bit? So it's not the same range exactly. I was wrong here, sorry.

Yes, I was about to explain that. Cool, if you find something else let me know. It has been tricky to test it extensively and you did a great job providing feedback.

I compared with a Redis implementation and couldn't find differences when start end arguments are given and identical regardless of bit/byte.

vazois marked this pull request as draft February 12, 2025 23:17

vazois force-pushed the vazois/bitpos-bitsearch branch 2 times, most recently from 2fbe7cd to 5dde961 Compare February 13, 2025 00:05

prvyk mentioned this pull request Feb 13, 2025

BITPOS bit search on the last byte should account for the byte's position. #1006

Closed

PaulusParssinen reviewed Feb 13, 2025

View reviewed changes

libs/server/Resp/Bitmap/BitmapManagerBitPos.cs Outdated Show resolved Hide resolved

vazois marked this pull request as ready for review February 13, 2025 19:31

vazois added 10 commits February 13, 2025 11:56

wip

a6a4755

cleanup bitpos tests

93f8031

Fix bitpos bit search

c4d86b3

simplify bitpos byte search

db835bc

cleanup unit tests for bitpos

ea0579c

fix masked invalidPayload matching

5578db4

add more tests

75368aa

fix boundary conditions for bit search and add tests

3666295

change to portable intrinsics

bb7ad0f

add more tests for bitpos

62ece05

vazois force-pushed the vazois/bitpos-bitsearch branch from a7574ea to 62ece05 Compare February 13, 2025 19:56

badrishc approved these changes Feb 13, 2025

View reviewed changes

vazois merged commit d7d2bc6 into microsoft:main Feb 14, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor BITPOS Implementation #1016

Refactor BITPOS Implementation #1016

vazois commented Feb 12, 2025

prvyk commented Feb 13, 2025 •

edited

Loading

vazois commented Feb 13, 2025

prvyk commented Feb 13, 2025

vazois commented Feb 13, 2025 •

edited

Loading

prvyk commented Feb 13, 2025 •

edited

Loading

vazois commented Feb 13, 2025 •

edited

Loading

prvyk commented Feb 14, 2025

Refactor BITPOS Implementation #1016

Refactor BITPOS Implementation #1016

Conversation

vazois commented Feb 12, 2025

prvyk commented Feb 13, 2025 • edited Loading

vazois commented Feb 13, 2025

prvyk commented Feb 13, 2025

vazois commented Feb 13, 2025 • edited Loading

prvyk commented Feb 13, 2025 • edited Loading

vazois commented Feb 13, 2025 • edited Loading

prvyk commented Feb 14, 2025

prvyk commented Feb 13, 2025 •

edited

Loading

vazois commented Feb 13, 2025 •

edited

Loading

prvyk commented Feb 13, 2025 •

edited

Loading

vazois commented Feb 13, 2025 •

edited

Loading