CSHARP-5670: Implement ReadOnlyMemoryBsonReader#1823
CSHARP-5670: Implement ReadOnlyMemoryBsonReader#1823BorisDog wants to merge 1 commit intomongodb:mainfrom
Conversation
BorisDog
added
the
improvement
There was a problem hiding this comment.
Pull request overview
This PR introduces ReadOnlyMemoryBsonReader, a new, optimized BSON reader implementation designed for continuous single-chunk buffers. The reader provides approximately 50% improvement in deserialization benchmarks compared to BinaryBsonReader.
Key changes:
- New
ReadOnlyMemoryBsonReaderclass that directly works withReadOnlyMemory<byte>instead of streams - New
ReadOnlyMemoryBufferimplementation ofIByteBufferfor read-only memory - Updated existing BSON deserialization code to use
BsonBinaryReaderUtils.CreateBinaryReader()which automatically selects the optimized reader when possible - Enhanced name decoders (
Utf8NameDecoder,TrieNameDecoder) to support span-based decoding - Various refactorings to reduce nesting and improve code readability
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ReadOnlyMemoryBsonReader.cs |
New optimized BSON reader implementation for contiguous memory |
ReadOnlyMemoryBuffer.cs |
New read-only byte buffer wrapper for ReadOnlyMemory<byte> |
ReadOnlyMemoryReaderSettings.cs |
Settings class for the new reader |
BsonBinaryReaderUtils.cs |
Factory utilities for creating appropriate reader implementations |
Utf8Helper.cs, Utf8NameDecoder.cs, TrieNameDecoder.cs |
Span-based decoding support |
RawBsonDocument.cs, RawBsonArray.cs, LazyBsonDocument.cs, LazyBsonArray.cs |
Refactored to use factory method for reader creation |
ObjectId.cs |
Refactored to use ReadOnlySpan<byte> for parsing |
BsonBinaryReader.cs |
Moved common utilities to BsonBinaryReaderUtils |
| Various operation files | Simplified using statements and refactored to use factory method |
Comments suppressed due to low confidence (1)
src/MongoDB.Bson/IO/ReadOnlyMemoryBsonReader.cs:1
- Double forward slashes in XML documentation comment. Should be
/// <param name="requiredBsonType">The required BSON type.</param>with three slashes, not four.
/* Copyright 2010-present MongoDB Inc.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BorisDog
requested review from
papafe and
rstam
and removed request for
ajcvickers
|
Assigned |
| get => _position; | ||
| set | ||
| { | ||
| if (value < 0 || value > _memory.Length) |
There was a problem hiding this comment.
Not available (yet?) in Bson lib.
| // insert the element name into the error message | ||
| var periodIndex = ex.Message.IndexOf('.'); | ||
| var dottedElementName = GenerateDottedElementName(); | ||
| var dottedElementName = BsonBinaryReaderUtils.GenerateDottedElementName(_context, _contextStack.ToArray(), () => _bsonStream.ReadCString(Utf8Encodings.Lenient)); |
There was a problem hiding this comment.
I'm pretty sure we have a context allocation here which could be avoided by passing _bsonStream as a parameter for GenerateDottedElementName. If the method is more generic, we should add TState parameter then.
There was a problem hiding this comment.
It's fine I think, it's not on a hot path.
For example _contextStack.ToArray() is also not ideal, but ok on an exception path.
| /// <summary> | ||
| /// Provides utility methods for working with <see cref="IBsonReader"/> instances in binary BSON format. | ||
| /// </summary> | ||
| public static class BsonBinaryReaderUtils |
There was a problem hiding this comment.
Unfortunately not, used in Driver.
| var dataSize = ReadSize(); | ||
| var totalSize = dataSize + 1; // data + subtype | ||
| var span = _memory.Span.Slice(_position, totalSize); | ||
| _position += totalSize; |
There was a problem hiding this comment.
Can we encapsulate memory slicing and position update in the helper methods?
private byte ReadByte() => _memory.Span[_position++];
and
private ReadOnlySpan<byte> ReadBytes(int size)
{
var span = _memory.Span.Slice(_position, size);
_position += size;
return span;
}
There was a problem hiding this comment.
I am hesitant about this for a couple of reasons:
- We won't be able to encapsulate all accesses to
_memoryand_positioninReadBytes, for example it does help with methods likeReadCStringFromMemory,ReadSliceFromMemory - I couldn't get it inlined easily
There was a problem hiding this comment.
Introduced IByteBufferSlicer for this.
| { | ||
| VerifyBsonType(BsonType.Array); | ||
|
|
||
| var slice = ReadSliceFromMemory(); |
There was a problem hiding this comment.
As far as I understood ReadSliceFromMemory does not copy bytes to a new buffer. I do not think it's safe to return reference to the original buffer. As RawBsonArray could outlive the original buffer and could still reference it even after it will be returned to pool or even reused.
There was a problem hiding this comment.
This is a good point, looking deeper into that.
| var bytes = new byte[] { 1, 2 }; | ||
| var subject = CreateSubject(bytes); | ||
|
|
||
| var e = Record.Exception(() => subject.Clear(0, 0)); |
There was a problem hiding this comment.
Can reuse ValidateWritableException here.
| private void ValidateWritableException(Action action) | ||
| { | ||
| var e = Record.Exception(action); | ||
| e.Should().BeOfType<InvalidOperationException>(); |
There was a problem hiding this comment.
Should it be NotSupportedException instead?
https://learn.microsoft.com/en-us/dotnet/api/system.notsupportedexception?view=net-10.0
There was a problem hiding this comment.
Aligning with ByteArrayBuffer here. We might reevaluate this in 4.0.
| { | ||
| var subject = CreateSubject(2); | ||
|
|
||
| Action action = () => subject.GetByte(position); |
There was a problem hiding this comment.
We are using Record.Exception usually.
| var subject = CreateSubject(2); | ||
| var destination = new byte[3]; | ||
|
|
||
| Action action = () => subject.GetBytes(position, destination, 0, count); |
There was a problem hiding this comment.
Record.Exception here and more below.
|
|
||
| Action action = () => subject.GetBytes(0, null, 0, 2); | ||
|
|
||
| action.ShouldThrow<ArgumentOutOfRangeException>(); |
There was a problem hiding this comment.
May be ArgumentNullException?
There was a problem hiding this comment.
Just propagating Span ctor exception for simplicity.
This PR introduces
ReadOnlyMemoryBsonReader, an alternative toBinaryBsonReader.ReadOnlyMemoryBsonReaderprovides a more lightweight BSON deserialization from a continuous single chunk buffers.Preliminary testing shows an improvement of ~50% in our deserialization benchmarks.