-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CSHARP-5666: Remove GetBitArray allocations in BsonClassMapSerializer.DeserializeClass #1767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good, just a small number of comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should choose a different name so there is no confusion with System.Collections.BitArray?
Not sure it's completely necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should rename it to
Deserialize_should_throw_FormatException_when_required_element_is_not_found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
By the way, have we tried benchmarking what is the difference in allocation on a test case?
src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. I have some minor suggestions. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, it's cleaner. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested refactoring to only pass the ArrayPool:
public static MembersBitArray GetMembersBitArray(int lengthInUInts) =>
new(lengthInUInts, ArrayPool<uint>.Shared);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mention that you could just use the existing allMemberMaps variable instead of _classMap.AllMemberMaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standardize capitalization as lengthInUInts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename length to lengthInUInts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename length to lengthInUInts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc3bf13 to
8e7608e
Compare
....DeserializeClass
8e7608e to
8e1e217
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.