[SRM] Optimize MetadataBuilder.GetOrAddConstantBlob.#121223
[SRM] Optimize MetadataBuilder.GetOrAddConstantBlob.#121223teo-tsirpanis wants to merge 6 commits intodotnet:mainfrom
MetadataBuilder.GetOrAddConstantBlob.#121223Conversation
The logic of writing scalar constants was extracted to its own method that writes to a span. This method also got used by `MetadataBuilder.GetOrAddConstantBlob`, avoiding the use of pooled blob builders when writing scalar constants.
|
Tagging subscribers to this area: @dotnet/area-system-reflection-metadata |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes constant blob encoding by eliminating unnecessary allocations. It refactors the GetOrAddConstantBlob method and the WriteConstant methods to use stack-allocated buffers instead of pooled BlobBuilder instances for scalar (non-string) constants.
Key Changes:
- Introduced a new
WriteScalarConstantmethod that writes scalar constants to aSpan<byte>and returns the number of bytes written - Refactored
GetOrAddConstantBlobto use stack allocation for scalar values - Updated both
WriteConstantoverloads to callWriteScalarConstantfor non-string values
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| MetadataBuilder.Heaps.cs | Refactored GetOrAddConstantBlob to use stack-allocated buffer and new WriteScalarConstant method |
| BlobWriterImpl.cs | Added new WriteScalarConstant method and refactored existing WriteConstant overloads to use it |
| /// <param name="bytes">The span where the content will be encoded.</param> | ||
| /// <param name="value">The constant value.</param> |
There was a problem hiding this comment.
The documentation should clarify that bytes must be at least sizeof(ulong) bytes in length to accommodate the largest scalar type. Additionally, it should document the expected behavior when value is null (returns 4 bytes for a zero uint).
| /// <param name="bytes">The span where the content will be encoded.</param> | |
| /// <param name="value">The constant value.</param> | |
| /// <param name="bytes"> | |
| /// The span where the content will be encoded. Must be at least <c>sizeof(ulong)</c> bytes in length to accommodate the largest scalar type. | |
| /// </param> | |
| /// <param name="value"> | |
| /// The constant value. If <paramref name="value"/> is <c>null</c>, writes 4 bytes representing a zero <c>uint</c>. | |
| /// </param> |
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriterImpl.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/BlobWriterImpl.cs
Show resolved
Hide resolved
AaronRobinsonMSFT
left a comment
There was a problem hiding this comment.
This looks good to me. Nothing obvious jumps out and I assume we have good test coverage for this area.
Do you have any perf numbers that show the benefit of this optimization? |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
|
I do not think the benchmark you have shared above is exercising the code changed in this PR. |
Oops, sorry; I should stop working on multiple PRs at the same time.
Yes; the idea is that serializing the blob to check if it exists would be faster, as it would use stack-allocated memory instead of a pooled |
|
Latest EgorBot results show a 2x slowdown for Alternatively, I'd be fine with optimizing just |
|
I had an idea on how to benchmark |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
| private void ClearBlobs() | ||
| { | ||
| _blobs.Clear(); | ||
| _blobs.GetOrAdd([], [], default, out _); |
...ibraries/System.Reflection.Metadata/src/System/Reflection/Metadata/Ecma335/BlobDictionary.cs
Outdated
Show resolved
Hide resolved
This reverts commit 3842337.
…tantBlob` is now optimized.
|
I copied the minimum necessary subset of SRM's sources into a separate benchmark project, and compared the old and new implementations of
I reverted all other parts of the PR and scoped it down to optimizing just |
BlobWriterImpl.WriteConstant.MetadataBuilder.GetOrAddConstantBlob.
In
MetadataBuilder.GetOrAddConstantBlob, the pooled blob builder was replaced by writing to a stack-allocated span, resulting in a 2x performance improvement when repeatedly writing the same scalar value (see comments in the bottom for benchmark results).