CSHARP-5666: Remove GetBitArray allocations in BsonClassMapSerializer.DeserializeClass #1767

BorisDog · 2025-09-01T18:19:40Z

No description provided.

….DeserializeClass

papafe

Overall it looks good, just a small number of comments.

papafe · 2025-09-02T08:13:11Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

        {
-            public static uint[] GetBitArray(int memberCount)
+            internal ref struct BitArray()


Maybe we should choose a different name so there is no confusion with System.Collections.BitArray?
Not sure it's completely necessary.

I think it's minor but still a valid point.
Changed to MemebersBitArray. Not sure about this naming though. Please suggest better alternatives if there are any.

papafe · 2025-09-02T08:42:29Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

-                bitArray[bitArrayLength] = ~0U << bitArrayOffset; // set unused bits to 1
-                return bitArray;
-            }
+                private readonly ArrayPool<uint> _arrayPool;


I can see you've started using ArrayPool here, are we going to use this instead of ThreadStaticBuffer from now on? I would agree to that.

I think so.
Would be good to have a quick benchmark to ensure there are no significant performance differences.

Another reason I didn't used ThreadStaticBuffer is because ThreadStaticBuffer is limited to byte type.

papafe · 2025-09-02T09:37:04Z

tests/MongoDB.Bson.Tests/Serialization/BsonClassMapSerializerTests.cs

+        [InlineData(1024, 993)]
+        [InlineData(1024, 1000)]
+        [InlineData(1024, 1023)]
+        public void Deserialize_should_throw_FormatException_when_no_required_element_is_found(int membersCount, int missingMemberIndex)


Maybe we should rename it to
Deserialize_should_throw_FormatException_when_required_element_is_not_found?

Thanks, done.

papafe · 2025-09-02T15:35:02Z

By the way, have we tried benchmarking what is the difference in allocation on a test case?

sanych-sun · 2025-09-08T17:05:08Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

+                return new(length, rentedBuffer, ArrayPool<uint>.Shared);
+            }
+
+            private static Span<uint> ResetSpan(Span<uint> span)


Can we use Span.Clear instead?

Thanks! Moved clearing to BitArray ctor for simplicity.

rstam

Looks really good. I have some minor suggestions. Let me know what you think.

rstam · 2025-09-09T20:55:45Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

-            var memberMapBitArray = FastMemberMapHelper.GetBitArray(allMemberMaps.Count);
+
+            var (bitArrayLength, useStackAlloc) = FastMemberMapHelper.GetMembersBitArrayLength(_classMap.AllMemberMaps.Count);
+            using var bitArray = useStackAlloc ? FastMemberMapHelper.GetMembersBitArray(stackalloc uint[bitArrayLength]) : FastMemberMapHelper.GetMembersBitArray(bitArrayLength);


I was confused by the name bitArrayLength because it sounded like it was the number of bits, but it's actually the number of uints.

I suggest some renaming:

var (lengthInUInts, useStackAlloc) = FastMemberMapHelper.GetLengthInUInts(allMemberMaps.Count); using var bitArray = useStackAlloc ? FastMemberMapHelper.GetMembersBitArray(stackalloc uint[lengthInUInts]) : FastMemberMapHelper.GetMembersBitArray(lengthInUInts);

Good idea, done.

rstam · 2025-09-09T20:58:05Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

                }
-                if ((bitBlock & 15) == 0)
+
+                public MembersBitArray(int spanLength, uint[] rentedBuffer, ArrayPool<uint> arrayPool) : this()


Instead of passing in BOTH the rentedBuffer and the arrayPool it came from how about just passing in the arrayPool?

public MembersBitArray(int lengthInUInts, ArrayPool<uint> arrayPool) : this() { _arrayPool = arrayPool; _rentedBuffer = _arrayPool.Rent(lengthInUInts); _bitArray = _rentedBuffer.AsSpan(0, lengthInUInts); _bitArray.Clear(); }

Also suggest renaming spanLength to lengthInUInts to standardize on a name that clearly indicates it is the length in uints and not the length in bits.

Good idea, it's cleaner. Done.

rstam · 2025-09-09T20:58:44Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

+            {
+                var length = (membersCount + 31) >> 5;
+                return (length, length <= 8); // Use stackalloc for up to 256 members
+            }


Some suggested renamings for clarity:

public static (int LengthInUInts, bool UseStackAlloc) GetLengthInUInts(int membersCoun { var lengthInUInts = (membersCount + 31) >> 5; return (lengthInUInts, lengthInUInts <= 8); // Use stackalloc for up to 256 members }

rstam · 2025-09-09T20:59:17Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

+                new(span);
+
+            public static MembersBitArray GetMembersBitArray(int length) =>
+                new(length, ArrayPool<uint>.Shared.Rent(length), ArrayPool<uint>.Shared);


Suggested refactoring to only pass the ArrayPool:

public static MembersBitArray GetMembersBitArray(int lengthInUInts) => new(lengthInUInts, ArrayPool<uint>.Shared);

rstam

Minor changes requested.

rstam · 2025-09-10T16:59:02Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

@@ -149,7 +151,9 @@ public TClass DeserializeClass(BsonDeserializationContext context)
            var discriminatorConvention = _classMap.GetDiscriminatorConvention();
            var allMemberMaps = _classMap.AllMemberMaps;
            var extraElementsMemberMapIndex = _classMap.ExtraElementsMemberMapIndex;
-            var memberMapBitArray = FastMemberMapHelper.GetBitArray(allMemberMaps.Count);
+
+            var (lengthInUInts, useStackAlloc) = FastMemberMapHelper.GetLengthInUInts(_classMap.AllMemberMaps.Count);


I forgot to mention that you could just use the existing allMemberMaps variable instead of _classMap.AllMemberMaps.

rstam · 2025-09-10T16:59:50Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

                }
-                if ((bitBlock & 15) == 0)
+
+                public MembersBitArray(int lengthInUints, ArrayPool<uint> arrayPool) : this()


Standardize capitalization as lengthInUInts?

rstam · 2025-09-10T17:00:09Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

+
+            public static (int LengthInUInts, bool UseStackAlloc) GetLengthInUInts(int membersCount)
+            {
+                var length = (membersCount + 31) >> 5;


Rename length to lengthInUInts?

rstam · 2025-09-10T17:00:32Z

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

+            public static MembersBitArray GetMembersBitArray(Span<uint> span) =>
+                new(span);
+
+            public static MembersBitArray GetMembersBitArray(int length) =>


Rename length to lengthInUInts?

CSHARP-5666: Remove GetBitArray allocations in BsonClassMapSerializer…

a7ff59a

….DeserializeClass

BorisDog requested a review from a team as a code owner September 1, 2025 18:19

BorisDog requested review from papafe and removed request for a team September 1, 2025 18:19

- Fix error message

ad91607

BorisDog added the improvement label Sep 1, 2025

BorisDog requested a review from rstam September 1, 2025 19:34

papafe previously approved these changes Sep 2, 2025

View reviewed changes

sanych-sun requested changes Sep 8, 2025

View reviewed changes

- PR comments

918ecac

BorisDog dismissed papafe’s stale review via 918ecac September 9, 2025 19:07

BorisDog requested review from sanych-sun and papafe September 9, 2025 19:08

rstam requested changes Sep 9, 2025

View reviewed changes

- PR comments

a913d58

BorisDog requested a review from rstam September 10, 2025 13:04

rstam requested changes Sep 10, 2025

View reviewed changes

rstam self-requested a review September 10, 2025 17:02

CSHARP-5666: Remove GetBitArray allocations in BsonClassMapSerializer.DeserializeClass #1767

Are you sure you want to change the base?

CSHARP-5666: Remove GetBitArray allocations in BsonClassMapSerializer.DeserializeClass #1767

Uh oh!

Conversation

BorisDog commented Sep 1, 2025

Uh oh!

papafe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BorisDog Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papafe commented Sep 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rstam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BorisDog Sep 9, 2025 •

edited

Loading