Skip to content

Conversation

@Henr1k80
Copy link
Contributor

@Henr1k80 Henr1k80 commented Oct 6, 2025

In #120336 I was suggested to add the improvements to the already existing ArrayBuilder<T> and use that in that PR.

The heap allocated buffer will only be allocated if capacity is above the 16 stack allocated capacity.

In cases where there is no conditional adds and capacity is known, it will not make a difference, as the previous internal buffer will just be returned.
But in these cases, an array with correct capacity and an index could just be used instead of the ArrayBuilder<T>.

I have removed the unused Buffer getter.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 6, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 6, 2025
internal struct ArrayBuilder<T>
{
private const int DefaultCapacity = 4;
private InlineArray16<T> _stackAllocatedBuffer = default;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anywhere ArrayBuilder<T> is used as a field on a class, this is going to increase the size of that class' allocation by 16 Ts.

Copy link
Contributor Author

@Henr1k80 Henr1k80 Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a problem.

But it is not used as a field currently. Could there be a warning in <remarks>, or is it a no go?
It could also be made a separate struct if it is useful? E.g. StackArrayBuilder<T>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could making it a ref struct could solve it for classes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private ArrayBuilder<TypeDesc> _typesThatNeedTypeHandles;
private ArrayBuilder<InstantiatedMethod> _methodsThatNeedDictionaries;
private ArrayBuilder<TypeDesc> _typesThatNeedPreparation;

Copy link
Contributor Author

@Henr1k80 Henr1k80 Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that shoots it down @jkotas, at least modifying ArrayBuilder<T>
I did not find these when opening sfx.slnx, so I guess there is more to it than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to make a ref struct StackArrayBuilder<T>?

@Henr1k80
Copy link
Contributor Author

Henr1k80 commented Oct 6, 2025

Oh, I can also see that some tests using reflection fails now, probably because of the Buffer get removal...

@Henr1k80
Copy link
Contributor Author

Henr1k80 commented Oct 6, 2025

Ok, I have changed it to a draft proposal of new stack allocating ref structs for building arrays.

One example of a simple fixed max stack size (8), where it fails hard, if you over go over the size.
It will not grow, but it is very simple. But it might need multiple size implementations.
But since it is internal, a size 13 could be made, for minimal stack allocation.

Another one, where growing is allowed, if max expected size is not known, or if growing, beyond expected capacity, is rare .
More complicated (branching-wise) though.
This can be used in the original PR #120336

I have written unit tests, but I do not know where to place them.
I guess this is the complicated part of adding new stuff, as I have seen errors from multiple other places when making a PR.
If you haven't guessed already, I am totally new to contributing to dotnet. Sorry for all the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants