Repository navigation
Clarify ArrayRecord handling of Multiple Null Records - #132479
Conversation
Co-authored-by: adamsitnik <[email protected]>
|
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. |
|
@adamsitnik I think the pertinent question is "why does this check exist?" It's purely a performance optimization, but it comes across as a correctness check. That's also why, in the conversation I had with you, I pushed back a bit against the unit test. Unit tests typically check correctness, and I think most readers and maintainers of the code base would not expect the presence of a performance-check-only unit test. (That deserves a comment in the unit test!) |
There was a problem hiding this comment.
Pull request overview
This PR updates the inline documentation above ArrayRecord.CheckExpectedRecordCount to clarify how NRBF array record counting behaves when “multiple null” records are present, especially when allowNulls is enabled.
Changes:
- Replaced the existing XML doc/remarks block with a shorter two-line comment describing the record-count vs element-count invariant.
- Focused the comment specifically on the special-case behavior of multiple-null records when nulls are allowed.
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
That's also why, in the conversation I had with you, I pushed back a bit against the unit test. Unit tests typically check correctness, and I think most readers and maintainers of the code base would not expect the presence of a performance-check-only unit test. (That deserves a comment in the unit test!)
I add test any time I want to make sure that given behavior/characteristic does not change over the time. We usually don't provide performance-oriented tests not because we don't want the performance to regress, but because it's really hard to implement such a test in a reliable way. That is why most of our performance tests live in the https://github.com/dotnet/performance repo.
In this case providing a test was straightforward: we don't care about CPU time, we just want to make sure it does not allocate a LOT of memory. Just so the check is not deleted in a few years from. Since that is unlikely to happen in dotnet/runtime I am OK with removing the test if this is very important to you @GrabYourPitchforks .
Co-authored-by: adamsitnik <[email protected]>
Removed the allocation-focused test in 3388e3e. |
|
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. |
No description provided.