In this meeting, we reviewed these two PRs:
These two PRs are essentially mutually exclusive.
The goal is to avoid the common case that creating an ImmutableArray<T> via a
builder causes multiple underlying arrays to be created.
In other words, the following code snippet should only create a single array:
var builder = ImmutableArray.CreateBuilder<int>(10);
for (int i = 0; i < builder.Capacity; i++)
{
builder.Add(i);
}
ImmutableArray<int> array = builder.ToImmutable();Today that's not the case. First, the builder doesn't actually store its
elements in a T[]. Instead, it uses a struct to avoid the covariance type
checks when writing to the array. This forces ToImmutable() to copy the
elements.
Another aspect is that calling ToImmutable() doesn't invalidate the builder.
In fact, it leaves all the data in the underlying builder and creates a new
array that is then used to construct the immutable array.
While that pattern can be convenient, it turns out that this has other problems. In particular, when using a pooling for builders:
-
All builders will eventually get promoted to Gen2. The bigger the data is that they hold, the more time the GC needs to scan Gen2.
-
Unless the builders are manually cleared, the pool will keep the contents of the builder alive.
Thus, the preferred behavior would be to expose an API that allows
-
To construct an
ImmutableArray<T>where the underlying array is created up-front and no additional buffers are needed. -
The builders can easily be pooled if necessary without causing the problems above.
This naturally yields to a model where the ownership of the underlying array
is essentially transferred to ImmutableArray<T>.
We looked at two approaches:
The first proposal adds a new builder. The second one just adds a new API on the existing builder.
We decided to go down with the second approach. Reasons:
- Using the existing builder with
Add()methods is more convenient, even if the size is already known. - Fewer concepts / types / APIs.
- Easier to migrate code between the two methods
The resulting experience will now look like this:
var builder = ImmutableArray.CreateBuilder<int>(10);
for (int i = 0; i < builder.Capacity; i++)
{
builder.Add(i);
}
ImmutableArray<int> array = builder.ExtractToImmutable();The API ExtractToImmutable() is designed to never allocate; this creates
issues if the Capacity doesn't match Count when ExtractToImmutable()
is called.
We decided that the best thing is to throw an InvalidOperationException.
The developer is responsible for making sure that the capacity is correct.
Alternatively, we could have allowed the capacity to be larger (for obvious
reasons it can't be smaller) and make ExtractToImmutable() simply use
the array as-is, i.e. with trailing null/zero elements. However, this could
create bugs down the road which is why we went with throwing an exception.
If the developer wants this behavior it's easy to do:
var builder = ImmutableArray.CreateBuilder<int>(2);
builder.Add(1);
builder.Add(2);
builder.Add(3);
builder.Count = builder.Capacity;
ImmutableArray<int> array = builder.ExtractToImmutable();In this case, the count is set to the current capacity. This doesn't cause any reallocations; it just means that the additional empty slots at the end of the array are now considered being part of the immutable array.
We need to define a name for this method. We don't like the current proposal
we lean toward MoveToImmutable() as it conveys the notion of transferring
ownership of the underlying array to ImmutableArray<T>.