-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Allocate exactly the space needed when creating a sourcetext #71886
Allocate exactly the space needed when creating a sourcetext #71886
Conversation
@@ -17,6 +17,10 @@ namespace Microsoft.CodeAnalysis.Shared.Extensions; | |||
|
|||
internal static partial class SourceTextExtensions | |||
{ | |||
// char pooled memory : 8K * 256 = 2MB | |||
private const int CharBufferSize = 4 * 1024; | |||
private static readonly ObjectPool<char[]> CharArrayPool = new(() => new char[CharBufferSize], 256); |
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.
moved these here as this is the only place it is used. and it is easier to validate proper usage.
src/Workspaces/Core/Portable/Shared/Extensions/SourceTextExtensions.cs
Outdated
Show resolved
Hide resolved
|
||
// number of chunks | ||
var numberOfChunks = 1 + (length / buffer.Length); | ||
writer.WriteInt32(numberOfChunks); | ||
|
||
// write whole chunks | ||
try |
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.
view with whitespace off.
var elementType = typeof(byte); | ||
Debug.Assert(s_typeMap[elementType] == TypeCode.UInt8); | ||
var elementType = typeof(T); | ||
Contract.ThrowIfFalse(s_typeMap.ContainsKey(elementType)); |
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.
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 don't mind this too much. so we get a nice contract failure if we defy expectations here.
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.
Not a big deal, but not sure why this is less clear:
Contract.ThrowIfFalse(s_typeMap.TryGetValue(elementType, out var actualType));
Contract.ThrowIfTrue(expectedType != actualType);
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.
ah, misunderstood. yes, that's nicer. will do in followup.
} | ||
|
||
/// <summary> | ||
/// Write an array of bytes. The array data is provided as a <see |
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.
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.
can fix in followup :)
Action<BinaryWriter, T[], int> write) | ||
{ | ||
var spanLength = span.Length; | ||
var buffer = System.Buffers.ArrayPool<T>.Shared.Rent(Math.Min(spanLength, rentLength)); |
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.
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.
offline: for straight IO buffers, this feels more in line with what the runtime does.
/// </summary> | ||
/// <param name="span">The array data.</param> | ||
public void WriteValue(ReadOnlySpan<byte> span) | ||
public void WriteSpan(ReadOnlySpan<byte> span) |
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.
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.
No description provided.