Add ReadOnlySpan constructors to BitArray - #131500
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4236475d-0243-488d-93d8-cd78e94d532b
|
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
Adds new ReadOnlySpan<T>-based constructors to System.Collections.BitArray to enable allocation-free construction from span-backed inputs, while preserving existing array-constructor behaviors by routing them through the new span overloads. This extends the public System.Collections contract and provides test coverage for span-specific scenarios (slices, stackalloc, copy semantics, and SIMD boundary safety).
Changes:
- Added
BitArray(ReadOnlySpan<bool>),BitArray(ReadOnlySpan<byte>), andBitArray(ReadOnlySpan<int>)constructors and routed existing array constructors through them. - Updated the
System.Collectionsreference assembly to expose the new public constructors. - Expanded unit tests to validate span parity with array constructors, slicing/stackalloc usage, source independence, non-canonical bool handling, and vector boundary safety.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Collections/BitArray.cs | Implements the new span constructors and reuses them from existing array constructors without changing established behaviors (null/overflow handling, packing semantics). |
| src/libraries/System.Collections/tests/BitArray/BitArray_CtorTests.cs | Adds coverage for span constructors (slices, stackalloc, copy semantics, non-canonical bools, and BoundedMemory OOB checks). |
| src/libraries/System.Collections/ref/System.Collections.cs | Adds the new public constructor signatures to the ref surface for System.Collections.BitArray. |
|
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. |
| /// copied from the specified read-only span of Booleans. | ||
| /// </summary> | ||
| /// <param name="values">A read-only span of Booleans to copy.</param> | ||
| public BitArray(ReadOnlySpan<bool> values) |
There was a problem hiding this comment.
Can you add the remarks from https://learn.microsoft.com/en-us/dotnet/api/system.collections.bitarray.-ctor?view=net-10.0#remarks? Also updating the array ctors to match the API docs would be good too.
As an aside, the way the documentation is synced is if a member has doc comments when it is introduced, it will be pulled into the api-docs repo (https://github.com/dotnet/dotnet-api-docs). If the doc comments are changed in the code again, they're not automatically synced to the api-docs repo and vice versa (changes in api-docs aren't automatically flowed back to code).
| /// <exception cref="ArgumentNullException"><paramref name="bytes"/> is null.</exception> | ||
| /// <exception cref="ArgumentException">The length of <paramref name="bytes"/> in bits is greater than <see cref="int.MaxValue"/>.</exception> | ||
| public BitArray(byte[] bytes) | ||
| : this(new ReadOnlySpan<byte>(bytes ?? throw new ArgumentNullException(nameof(bytes)))) |
There was a problem hiding this comment.
nit: While this pattern is convenient, it is also expensive and can hinder inlining
This likely warrants the code duplication or deference to a central initialize helper after the validation is done so we can keep using the throw helper.
There was a problem hiding this comment.
you can notably do something like this if you want to keep it simple and it should avoid pessimizing things too much (bitarry is small so the worst case of a small copy is better than not inlining)
ArgumentNullException.ThrowIfNull(bytes);
this = new BitArray(bytes.AsSpan());There was a problem hiding this comment.
nit: While this pattern is convenient, it is also expensive and can hinder inlining
This likely warrants the code duplication or deference to a central initialize helper after the validation is done so we can keep using the throw helper.
I considered this but I assumed that people that wanted the perf would use the ROS overload (but the caller would explicitly need to convert existing call sites). How much impact does this have on inlining?
you can notably do something like this if you want to keep it simple and it should avoid pessimizing things too much (bitarry is small so the worst case of a small copy is better than not inlining)
ArgumentNullException.ThrowIfNull(bytes); this = new BitArray(bytes.AsSpan());
This syntax is only valid for structs right?
There was a problem hiding this comment.
How much impact does this have on inlining?
Fairly significant and you can often presume that it means it won't happen. It's not a strict guarantee, but it adds a fairly likely blocker.
I assumed that people that wanted the perf would use the ROS overload
Most users aren't thinking about perf, so we need to think about the basics for them (at least where trivial like this). If a user already has an array, they're unlikely to insert an explicit AsSpan() for example
There was a problem hiding this comment.
I think the simplest option is the centralized helper method, i.e. CreateArray? I think Pranav is right, the aforementioned syntax is only available for structs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4236475d-0243-488d-93d8-cd78e94d532b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4236475d-0243-488d-93d8-cd78e94d532b
Summary
ReadOnlySpan<bool>,ReadOnlySpan<byte>, andReadOnlySpan<int>constructors toBitArray.Fixes #80263
Validation
BitArray_CtorTestspassed with default hardware intrinsicsBitArray_CtorTestspassed withDOTNET_EnableAVX2=0BitArray_CtorTestspassed withDOTNET_EnableHWIntrinsic=0System.Collectionstests passedPerformance
Full BenchmarkDotNet results from Windows ARM64 Release:
span.ToArray()workaround across the measured sizes.Note
This pull request description was generated with GitHub Copilot.