*** WIP: Create PooledSpanBuilder, strongly based on PooledArrayBuilder #12095
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| } | ||
|
|
||
| return GetInlineElement(index); | ||
| return _array[index]; |
There was a problem hiding this comment.
Feels like the indexer should still validate against count, since the array length could be larger and therefore the underlying indexer can't be relied on to throw.
| /// continue to be used, even if the arrays shrinks and has fewer than 4 elements. | ||
| /// Wraps a pooled array but doesn't allocate it until it's needed. Provides | ||
| /// access to the backing array as a <see cref="Span{T}"/>. | ||
| /// Note: Disposal ensures the pooled array is returned to the pool. | ||
| /// </summary> | ||
| [NonCopyable] |
There was a problem hiding this comment.
Is it worth adding some notes about capacity behaviour? ie adding could resize and cause an array copy, and that nothing fancy is done to prevent LOH etc. so there are better choices for really big arrays. Though I might just be being a bit paranoid :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
*** Note: This is in draft mode until I receive verification that I'm not missing a data structure that satisfies all my span and pooling desires. Also, I'm on vacation until the 18th, so I'm creating this before my trip in the hopes of having juicy feedback when I get back.
Insertion PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/660373
I've found myself in need of a pooled array data structure that gives me access to an underlying span.
I've considered:
-- Doesn't have an underlying array for <= 4 elements.
-- Backing List doesn't provide access to the underlying array (it does in Net5 and higher though)
Perhaps there is another data structure which can do this, in which case the majority of this PR can be nuked. If not, it was fairly easy to augment the PooledArrayBuilder class to rip out the inline elements.
This is in response to some allocations I'm seeing in the MetadataBuilder and MetadataCollection classes in the razor speedometer cohosting completion test. This change allows those classes to work better with spans, avoiding allocations.
Best reviewed by commit:
Commit 1: Change PooledArrayBuilder to PooledSpanBuilder
Commit 2: Move PooledSpanBuilder implementation to appropriately named files
Commit 3: Add back the PooledArrayBuilder in all it's beauty
Commit 4: Tests (thanks copilot!)
Commit 5: Add in product usage of the new classes
*** Before changes ***
*** With changes, results from speedometer run linked above ***
