Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[hackathon] [testonly] ValueArray - a compliment type to the Span, which owns its data without indirections. #60608

Closed
wants to merge 24 commits into from

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Oct 19, 2021

[NO MERGE]

Same Changes as in #60519 - just to run tests with everything and experiment with using the feature in corlib and libraries.

This is not going into Main. :-)
I am just using this PR to experiment with ValueArray in the runtime/libraries and to run tests.


ValueArray<T, Size> is a data type that represents an indexable fixed-sized chunk of data containing Size elements of type T.
The Size type argument must be an object array type. It is not used for anything other than to convey the length of the value array declaratively - via the Size's rank.

The only "magical" part of the ValueArray type is that the VM logically replicates the storage N times and adjusts the instance layout and GC tracking accordingly.

In all other ways this is a regular struct - it can be copied by value, embedded in another struct/class, passed in/out of methods, stored on the heap, boxed...


With some Roslyn work, it should be possible to put a nice syntax on value arrays.

Just like one does not need to spell out System.Nullable<T> and can write T? instead, it is possible to have nice syntax for value arrays, for example T[length] could map to ValueArray<T, object[,, ~ ,,]> where the rank of the marker object array matches the desired length.

Such syntax would allow writing code like the following:

class QuadTree
{
    public readonly int Depth;
    private QuadTree[4] _nodes;  //field

    public ref readonly QuadTree[4] Nodes => ref _nodes;  // ref return type

    public int[4] DepthMask()    // byval return type
    {
        int[4] mask = default;   // local variable
        for(int i = 0; i < mask.Length; i++)
        {
            mask[i] = _nodes[i].Depth;
        }

        return mask;
    }
}

@VSadov VSadov added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Oct 19, 2021
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

public int Length => RankOf<R>.Value;

// For the array of Length N, runtime will add N-1 elements immediately after this one.
private T Element0;
Copy link
Member

Choose a reason for hiding this comment

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

This would mean ValueArray is compatible with fixed-sized buffers and is blittable across P/Invoke boundaries, correct?

The debugger and P/Invoke have sometimes had issues with fixed-sized buffers since they don't actually contain N elements; so there may be additional work there required (before this goes to main, not necessarily for the hackathon branch) to ensure its handled correctly. This would presumably include handling for CallConvMemberFunction where Windows treats primitives and struct wrappers differently.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not going into Main. :-)
I am just using this PR to run tests. The hackathon branch is not wired for that.

Copy link
Member Author

@VSadov VSadov Oct 19, 2021

Choose a reason for hiding this comment

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

I do not see fundamental reasons why BlittableType[length] is not blittable itself.

I did not do anything special for that though. I think such types will be classified as blittable, - just as if ValueArray<T, Size> had only one field and the T is blittable.
The type also knows its unmanaged size. - i.e. sizeof(BlittableType[length]) works correctly and tested in the tests.

Whether that is enough to go safely across PInvoke boundary - maybe. :-)
I just do not know that area well enough to tell. I would not be surprised if some, hopefully minor, thing is missing.

I am not even sure how to properly test that area.
What tests would be needed?

Copy link
Member

Choose a reason for hiding this comment

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

This is not going into Main

Right, that was the reason for me saying "before this goes to main, not necessarily for the hackathon branch"

Just to be clear, I don't think anything needs to be done in this PR. I was trying to ask a couple questions about the overall implementation and list considerations that might be needed if/when this is productized and we look at officially supporting such a feature.

I do not see fundamental reasons why BlittableType[length] is not blittable itself.

I was more trying to confirm that the layout of ValueArray<int, 5> is equivalent to int data[5]; in C/C++ (that is, its only 5 sequential elements of type int, there is no length prefix/postfix or other state carried).

What tests would be needed?

We have a number of existing tests covering blittable generics here: https://github.com/dotnet/runtime/tree/main/src/tests/Interop/PInvoke/Generics. @AaronRobinsonMSFT and @jkoritzinsky would be the people to loop in on the overall discussion if/when this becomes more than just a hackathon prototype.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was more trying to confirm that the layout of ValueArray<int, 5> is equivalent to int data[5]; in C/C++ (that is, its only 5 sequential elements of type int, there is no length prefix/postfix or other state carried).

That is correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

The type's layout is the same field repeated N times.
If the field of type T is tightly packed, then the whole thing is tightly packed.

It is basically like the body of an array of T, but without storing length in the instance - since that is statically known as a part of the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, I noticed that JIT is very good at eliding and hoisting range checks.

Since the length is statically known, arr[4] will omit the range check if the Length is 5.
Loops like for(int i = 0, i < arr.Length; i++) {DoSomething(arr[i]); } will not have range checks either.

I was thinking that these could be good optimizations, but it looks like JIT does that without any additional work.

Copy link
Member

Choose a reason for hiding this comment

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

We will may need to add special support in the runtime to validate that the "non-blittable struct containing a blittable ValueArray<T, A> field" scenario is marshalled correctly. This was the scenario that was broken for fixed-buffers until .NET Core 3.0. This isn't super important with the hackathon branch, but should be considered if we ever take something of this form into the product.

Comment on lines 365 to 369
#if NICE_SYNTAX
private readonly T[]?[MaxBuffersPerArraySizePerCore] _arrays;
#else
private readonly ValueArray<T[]?, object[,,,,,,,]> _arrays;
#endif
Copy link
Member

@jkoritzinsky jkoritzinsky Oct 21, 2021

Choose a reason for hiding this comment

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

This block makes me think, could we make this experience much smoother with a C# language feature for numeric generic type parameters that could reuse various portions of the ArrayShape encoding structure? That way, this code could be a little cleaner and actually use the constant in the ValueArray case.

It's probably worth a proposal if there isn't one already.

Copy link
Member Author

@VSadov VSadov Oct 21, 2021

Choose a reason for hiding this comment

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

There is a Roslyn hackathon PR for a better C# syntax - dotnet/roslyn#57286
It would allow using constant here.

Since this is against the Main branch, I have to refer to the underlying type as-is.
(that is like spelling System.Nullable<type> instead of type?)

With corresponding Roslyn changes I could do:
image

(NOTE: the tooltip reveals the actual underlying type. Visualizing as Type[length] is NYI in the prototype)

Copy link
Member

Choose a reason for hiding this comment

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

I did not know about the Roslyn-side of this hackathon project. That's great!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll keep this unresolved - in case someone else has the same question.

@VSadov VSadov force-pushed the fixArrHacksMain branch 3 times, most recently from 9cff6d4 to b0a8b3e Compare October 22, 2021 07:44
@VSadov VSadov force-pushed the fixArrHacksMain branch 7 times, most recently from b344f89 to 342ebbe Compare October 23, 2021 16:21
@VSadov
Copy link
Member Author

VSadov commented Nov 26, 2021

this PR no longer needs to be open

@VSadov VSadov closed this Nov 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-api-needs-documentation NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants