Improve NRBF ArrayRecord.GetArray(allowNulls: false) perf - #132293
Improve NRBF ArrayRecord.GetArray(allowNulls: false) perf#132293adamsitnik with Copilot wants to merge 2 commits into
Conversation
… early when null records are detected Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR optimizes NRBF array materialization in System.Formats.Nrbf for the allowNulls: false path by detecting the presence of compressed “multiple null” records up-front and throwing before allocating and/or iterating large arrays.
Changes:
- Add an early
LengthvsRecords.Countguard in SZ-array record implementations to throw immediately when aNullsRecordwithNullCount > 1must be present andallowNullsisfalse. - Add an analogous early
destination.LongLengthvssource.Countguard inArrayRecord.Populateto avoid iterating large multidimensional/jagged arrays whenallowNullsisfalseand compressed nulls are present. - Add a regression test ensuring
GetArray(allowNulls: false)does not trigger a massive allocation for a payload usingObjectNullMultiple.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs | Early throw on allowNulls:false when record count cannot match element count due to multi-null compression. |
| src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs | Same early-throw optimization for string SZ arrays. |
| src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SZArrayOfRecords.cs | Same early-throw optimization for SZArrayRecord<SerializationRecord> materialization. |
| src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArrayRecord.cs | Early throw in Populate to avoid expensive iteration when allowNulls:false and compressed nulls are present. |
| src/libraries/System.Formats.Nrbf/tests/ArrayOfSerializationRecordsTests.cs | Add allocation-guard regression test for GetArray(allowNulls:false) with ObjectNullMultiple. |
adamsitnik
left a comment
There was a problem hiding this comment.
@copilot address my feedback
| internal static void Populate(List<SerializationRecord> source, Array destination, int[] lengths, AllowedRecordTypes allowedRecordTypes, bool allowNulls) | ||
| { | ||
| // When destination length is different than record count, we know the record list contains at least one Multiple Null Record. | ||
| if (!allowNulls && destination.LongLength != source.Count) |
There was a problem hiding this comment.
@copilot The destination array is already provided here which means it is already allocated. This check must be performed BEFORE the array gets allocated.
There was a problem hiding this comment.
Moved the check out of Populate into RectangularArrayRecord.Deserialize and JaggedArrayRecord.Deserialize, where it now runs before Array.CreateInstance/new ArrayRecord[...]. The shared logic lives in ArrayRecord.ThrowIfNullsAreNotAllowedButPresent, which compares ArrayInfo.FlattenedLength against the record count. bbc5c73
|
|
||
| WriteSerializedStreamHeader(writer); | ||
|
|
||
| writer.Write((byte)SerializationRecordType.ArraySingleObject); |
There was a problem hiding this comment.
@copilot Make this test a [Theory] and pass the array type byte as an argument. Make sure you cover not just string array, but also object array and other reference type array (I don't remember the exact record type name, but you modified three SZArray record types and they all need to be covered with tests without code duplication).
There was a problem hiding this comment.
Converted to a [Theory] over SerializationRecordType, covering ArraySingleObject (ArraySingleObjectRecord), ArraySingleString (ArraySingleStringRecord) and BinaryArray with a SystemClass element type (SZArrayOfRecords). Verified all three cases fail without the product change and pass with it. bbc5c73
… types with a Theory Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Performance optimization: when
GetArray(allowNulls: false)is called, short-circuit by comparingLengthagainstRecords.Countbefore allocating the result array. If they differ, a Multiple Null Record is present and we can throw immediately — avoiding a potentially massive allocation.Changes
ArraySingleObjectRecord.ToArray— early length vs record count check before array allocationArraySingleStringRecord.ToArray— sameSZArrayOfRecords.ToArray— sameArrayRecord.Populate— earlydestination.LongLengthvssource.Countcheck before iterating (coversRectangularArrayRecordandJaggedArrayRecord)ArrayOfSerializationRecordsTests— added test verifying no large allocation occurs whenGetArray(allowNulls: false)is called on a payload containingObjectNullMultipleExample