Reduce allocations during checksum creation. #76524
Conversation
Initially creating this PR as a draft PR to get speedometer results to see if this actually improves things. From the csharp editing scrolling speedometer, I see about 0.3% of total allocations in the VS process occurring in Checksum.Create. A large part of these allocations are stream/writer constructions. Instead, this PR attempts to use a small pool to reuse and reset these objects.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
Test insertion PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/599592 Speedometer results look good, elevating out of draft status |
|
@CyrusNajmabadi -- ptal |
|
What have you done for me lately?? |
| @@ -22,6 +22,8 @@ internal readonly partial record struct Checksum | |||
|
|
|||
| private static readonly ObjectPool<XxHash128> s_incrementalHashPool = | |||
| new(() => new(), size: 20); | |||
| private static readonly ObjectPool<ObjectWriter> s_objectWriterPool = | |||
| new(() => new(SerializableBytes.CreateWritableStream(), leaveOpen: true, writeValidationBytes: true), size: 4); | |||
There was a problem hiding this comment.
It's because the object is used for such a short period, that it's actually pretty unlikely to have concurrent usage of different items from the pool. I'll add a comment indicating such.
| using var stream = SerializableBytes.CreateWritableStream(); | ||
| // Obtain a writer from the pool | ||
| var objectWriter = s_objectWriterPool.Allocate(); | ||
| var stream = objectWriter.BaseStream; |
There was a problem hiding this comment.
consider moving this till after writing out hte object.
| var newChecksum = Create(stream); | ||
|
|
||
| // Reset object writer back to it's initial state | ||
| objectWriter.Reset(); |
There was a problem hiding this comment.
does writeValidationBytes then work properly? do we basically check if we're at pos0 and wrrite out those bytes (i genuinely don't remember).
There was a problem hiding this comment.
Changed in commit 2 to just rewrite the validation bytes after the reset instead of having the reset set the position to after the validation bytes. Probably easier to understand that way.
| @@ -129,6 +129,19 @@ public void Dispose() | |||
| public void WriteUInt16(ushort value) => _writer.Write(value); | |||
| public void WriteString(string? value) => WriteStringValue(value); | |||
|
|
|||
| public Stream BaseStream => _writer.BaseStream; | |||
|
|
|||
| public void Reset(bool includeValidationBytes = true) | |||
There was a problem hiding this comment.
hrmm... why is this optional. that seems... onfusing.
There was a problem hiding this comment.
no one seems to ever call this with a value. so just remove and always include the bytes?
There was a problem hiding this comment.
Yeah, probably shouldn't have worried about supporting that case
| public void Reset(bool includeValidationBytes = true) | ||
| { | ||
| _stringReferenceMap.Reset(); | ||
| _writer.BaseStream.Position = includeValidationBytes ? 2 : 0; |
There was a problem hiding this comment.
might want to doc this. perhaps that the bytes are already there, and we want to move after it.
alterantively, hard reset to 0 and write the bytes again.
| if (_writer.BaseStream is SerializableBytes.ReadWriteStream pooledStream) | ||
| pooledStream.SetLength(_writer.BaseStream.Position, truncate: false); | ||
| else | ||
| _writer.BaseStream.SetLength(_writer.BaseStream.Position); |
There was a problem hiding this comment.
def find this whole part unclear. considering doc'ing.
…fter them Added some comments to clarify
From the csharp editing scrolling speedometer, I see about 0.3% of total allocations in the VS process occurring in Checksum.Create. A large part of these allocations are stream/writer constructions. Instead, this PR attempts to use a small pool to reuse and reset these objects.
*** before image showing % of VS allocations due to Checksum.Create ***
*** after image showing % of VS allocations due to Checksum.Create ***
*** before image showing types allocated under Checksum.Create ***
*** after image showing types allocated under Checksum.Create ***
