Adds UnsafeCollectionOperations for unsafe access to RepeatedField<T>

[PascalSenn]

This is a proposal to add UnsafeCollectionOperations for fast access on RepeatedField<T>
#16745

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[jskeet]

@JamesNK We're aware that if the repeated field is modified while the span is "active", it may or may not reflect the changes (e.g. add/remove operations will not be seen in terms of the length, and if the array is reallocated any further updates won't be visible at all) - but do you think that's reasonable behavior, so long as we document that appropriately? (I'd just say that the state of the span is undefined after any modifications.) Any other thoughts about this PR?

[JamesNK]

Modification is the problem. Sometimes the span reflects changes, sometimes it doesn't.

I like the idea, but I don't think it should be a property on RepeatedField<T>. It makes the dangerous behavior too easy to access. I suggest doing what the BCL does with List<T> + span and make it a static method on an "advanced" type.

https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.collectionsmarshal.asspan?view=net-8.0

A property is easily discoverable with autocomplete. On the other hand, a method on a type is something people have to go looking to use, and they'll hopefully read the docs about why they should (or shouldn't) use the method.

foreach (var product in CollectionsMarshal.AsSpan(response.Products))
{
    // do stuff
}

I took a look at RepeatedField<T> source code and setting values does nothing more than add the value to the array (with various safety checks). The AsSpan method could return a Span<T> so it can also be used as a fast way to modify the repeated field as well.

var request = new Request();
CollectionsMarshal.SetCount(request.Products, count: 1000);
var productsSpan = CollectionsMarshal.AsSpan(request.Products);

for (var i = 0; i < productsSpan.Length; i++)
{
    productsSpan[i] = // do stuff
}

This would save many internal array allocations and copies as the repeated field grows.

However, SetCount (a new version for repeated fields) + modifying the span directly wouldn't guarantee the values in the repeated field aren't null.

[PascalSenn]

@JamesNK @jskeet

I changed the implementation to include your suggestions. There is already a similar class UnsafeByteOperations, so I aligned the naming with it -> UnsafeCollectionOperations. The implementation of the class is very close to the CollectionsMarshal.cs from System.Runtime.InteropServices. I implemented SetCount and AsSpan. I also ported the tests over from CollectionsMarshalTests.cs.

[jskeet]

I haven't reviewed the tests in detail as I'm suggesting quite significant implementation changes.

[PascalSenn]

@jskeet
Most of this implementation closely aligned with CollectionsMarshal.cs from the runtime. The goal was to keep the implementation and behavior relatively consistent.

I also believe a few things were performance optimizations in the CollectionsMarshal. For example, the aggressive inlining and the direct field access.

I incorporated your feedback into this pull request to better align with the behavior of Protobuf. I left a couple of comments open to provide some context. Feel free to close them as needed after review, as everything should be resolved.

[PascalSenn]

@jskeet One last remark. The tests are ported from CollectionsMarshalTests.cs to ensure consistent behavior:
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.InteropServices/tests/System.Runtime.InteropServices.UnitTests/System/Runtime/InteropServices/CollectionsMarshalTests.cs

[jskeet]

I'll have a look tomorrow. I would say that I'm not as interested in alignment with CollectionsMarshal as with consistency with the rest of this codebase, and clarity of code and usage.

[JamesNK]

I think its good! Some minor comment and test improvements in feedback

[JamesNK]

How come net462 is removed? I think it's important to run tests on .NET Framework. If some features aren't supported on .NET Framework then they can be selectively #ifdefed out.

[PascalSenn]

Oops. I removed it to be able to run the test on my unix machine

[jskeet]

Down to just nits now - it feels like we're very close, thanks very much for this!

[jskeet]

cc @amanda-tarafa for any additional thoughts

[amanda-tarafa]

A couple of comments, but looks good otherwise.

[PascalSenn]

@JamesNK @jskeet @amanda-tarafa
I think i resolved all the suggestions. Feel free to have another look

[jskeet]

Thanks - I'm afraid at this point I won't get to this until Monday, but hopefully I'll be able to look then.

[jskeet]

Just down to comment nits from my side.

[PascalSenn]

@jskeet Comments are resolved 🚀

[jskeet]

@JamesNK: Could you take one final pass at this? It looks good to me, but due to the unsafe aspects of it, I'd appreciate a double-check. After that, I can get it reviewed by a member of the protobuf team and merged.

[jskeet]

@mkruskal-google: We've reviewed this from a .NET perspective. Let me know if you'd like any more background. (But basically this allows very efficient access to a repeated field, with caveats around anything that might resize it.)