CSHARP-5611: Eliminate the temporary byte array allocation in ObjectId.ToString#1706
CSHARP-5611: Eliminate the temporary byte array allocation in ObjectId.ToString#1706ugurpelister wants to merge 2 commits intomongodb:mainfrom
Conversation
|
I think like the previous two PRs for this there this is going to regress on big endian for very real-world gains. |
Specifically comparing to the existing implementation of |
| { | ||
| if (destination.Length < 12) | ||
| { | ||
| throw new ArgumentException("Not enough room in destination span.", "destination"); |
There was a problem hiding this comment.
I know that this is against the existing convention in the file, but for new methods we use should use nameof(...).
| } | ||
|
|
||
| #if NET6_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER | ||
| /// <summary> |
There was a problem hiding this comment.
There is no need for #if, we include the System.Memory package.
| throw new ArgumentException("Not enough room in destination span.", "destination"); | ||
| } | ||
|
|
||
| Span<byte> span = stackalloc byte[12]; |
There was a problem hiding this comment.
I think we should reuse the existing ToString implementation, to avoid the Span allocation (even thought it's on stack) and for easier review.
While ToString() should just fallback to ToCharSpan, as it does already, but for all TFMs.
| public override string ToString() | ||
| { | ||
| #if NET6_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER | ||
| return string.Create(24, this, static (span, input) => |
There was a problem hiding this comment.
nit: expression-bodied method
| /// </summary> | ||
| /// <param name="destination">The destination byte span.</param> | ||
| /// <exception cref="ArgumentException">Not enough room in destination span.</exception> | ||
| public void ToByteSpan(Span<byte> destination) |
There was a problem hiding this comment.
For completeness, please add internal ObjectId(ReadOnlySpan<byte> span) contstructor
And appropriate tests.
| #if NET6_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER | ||
| Span<char> span = new char[24]; | ||
| objectId.ToCharSpan(span); | ||
| Assert.True(span.SequenceEqual("0102030405060708090a0b0c")); |
There was a problem hiding this comment.
Please add separate tests for the new methods, testing different cases:
ToByteSpan_should_return_expcted_results(byte[] data, byte[] expectedBytes)
ToCharSpan_should_return_expected_results(byte[] data, string expectedChars) (this one can also test Tostring)
3 participants
ObjectId's
ToStringmethod is among the highest of all highly frequently used methods in this library. The temporary byte array allocation that takes place insideToStringis an unnecessary one that adds workload on the GC.Given that the same functionality can be achieved by a leaner implementation for .NET6+ and .NETStandard2.1, we propose a Span-based implementation for these two platforms so that the resulting string can by created without leaving any garbage. The old implementation can continue to be used on .NET Framework 4.7.2.