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

[API Proposal]: Create array from array type #88620

Merged
merged 26 commits into from
Nov 16, 2023

Conversation

AlexRadch
Copy link
Contributor

@AlexRadch AlexRadch commented Jul 10, 2023

For #76478

I added InternalCreateFromArrayType method. InternalCreateFromArrayType method is only correctly implemented for the NativeAot platform.

At CoreCLR and Mono platforms it needs to be rewritten. At CoreCLR and Mono it calls InternalCreate method which slows it down.

The implementer is encouraged to consider whether all of these overloads are actually required, as the suggestion that the first overload will be in excess of 99% of all calls seems correct.

I decided to implement only the next overloads:

namespace System;

public partial class Array
{
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params int[] lengths);
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int[] lengths, int[] lowerBounds);
}

And I have not implemented the following overloads:

namespace System;

public partial class Array
{
        // It is not used in the current code and overload with `params int[] lengths` can be used instead
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2); 

        // It is not used in the current code and overload with `params int[] lengths` can be used instead
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, int length1, int length2, int length3);

        // Array does not support lengths more than `int.Max`. Array always uses `int` for lengths, not `long`
        public static System.Array CreateInstanceFromArrayType(System.Type arrayType, params long[] lengths);
}

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Jul 10, 2023
@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, 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.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 10, 2023
@huoyaoyuan
Copy link
Member

And I have not implemented the following overloads:

They are approved for consistency with current overloads.

It is not used in the current code and overload with params int[] lengths can be used instead

It should be somehow useful in third party code. Params array requires allocation, so we typically define overloads for two or three parameters.

If you have concerns about these overloads, you can provide feedback in original issue.

@AlexRadch
Copy link
Contributor Author

They are approved for consistency with current overloads.

They wrote

The implementer is encouraged to consider whether all of these overloads are actually required, as the suggestion that the first overload will be in excess of 99% of all calls seems correct.

@AlexRadch
Copy link
Contributor Author

It is not used in the current code and overload with params int[] lengths can be used instead

It should be somehow useful in third party code. Params array requires allocation, so we typically define overloads for two or three parameters.

Methods with two or three parameters will allocate an array inside to call InternalCreate method. So it does not reduce allocations.

@huoyaoyuan
Copy link
Member

Methods with two or three parameters will allocate an array inside to call InternalCreate method.

They shouldn't. CreateInstance uses stackalloc.

@AlexRadch
Copy link
Contributor Author

They shouldn't. CreateInstance uses stackalloc.

You are right, CreateInstance uses stackalloc. These methods are so rarely used .Net library does not use them at all. Can the compiler also use stack allocation? I think they are actually NOT required.

@huoyaoyuan
Copy link
Member

Can the compiler also use stack allocation?

That's params Span designed for (dotnet/csharplang#1757). The prerequisite work has started, and I'd expect that feature to come in C# 12 or 13.

@AlexRadch
Copy link
Contributor Author

That's params Span designed for (dotnet/csharplang#1757). The prerequisite work has started, and I'd expect that feature to come in C# 12 or 13.

So method overloads with two and three params are NOT required. Later we will add params Span overload and it will not allocate memory.

{
if (rank == 1 && !ContainsLowerBounds(rank, pLowerBounds))
{
return GC.AllocateNewArray(arrayType.TypeHandle.Value, pLengths[0], GC.GC_ALLOC_FLAGS.GC_ALLOC_NO_FLAGS);
Copy link
Member

Choose a reason for hiding this comment

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

GC.AllocateNewArray is designed for allocating the special arrays. You may want to measure how slow it is for allocating ordinary small arrays.

We may want to have a new runtime calls for this that will handle the multi-dim case too

Copy link
Contributor Author

@AlexRadch AlexRadch Jul 13, 2023

Choose a reason for hiding this comment

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

I got the next benchmark scores: https://github.com/AlexRadch/Temp/tree/GC_AllocateNewArray_Bench

|             Type |                Method |   Count |             Mean |          Error |         StdDev |           Median | Ratio | RatioSD |     Gen0 |     Gen1 |     Gen2 | Allocated | Alloc Ratio |
|----------------- |---------------------- |-------- |-----------------:|---------------:|---------------:|-----------------:|------:|--------:|---------:|---------:|---------:|----------:|------------:|
|    BenchArrayInt |                Create |       0 |         8.531 ns |      0.0618 ns |      0.0548 ns |         8.537 ns |  1.00 |    0.00 |   0.0057 |        - |        - |      24 B |        1.00 |
|    BenchArrayInt |              Allocate |       0 |        45.232 ns |      0.9173 ns |      1.2245 ns |        44.796 ns |  5.36 |    0.18 |   0.0057 |        - |        - |      24 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized |       0 |         8.971 ns |      0.0736 ns |      0.0652 ns |         8.985 ns |  1.05 |    0.01 |   0.0057 |        - |        - |      24 B |        1.00 |
| BenchArrayString |                Create |       0 |         8.193 ns |      0.1971 ns |      0.3554 ns |         8.040 ns |  0.96 |    0.04 |   0.0057 |        - |        - |      24 B |        1.00 |
| BenchArrayString |              Allocate |       0 |        46.420 ns |      0.9399 ns |      1.4353 ns |        46.031 ns |  5.51 |    0.19 |   0.0057 |        - |        - |      24 B |        1.00 |
| BenchArrayString | AllocateUninitialized |       0 |         7.978 ns |      0.1903 ns |      0.1869 ns |         7.957 ns |  0.94 |    0.02 |   0.0057 |        - |        - |      24 B |        1.00 |
|                  |                       |         |                  |                |                |                  |       |         |          |          |          |           |             |
|    BenchArrayInt |                Create |       1 |        10.022 ns |      0.0600 ns |      0.0532 ns |        10.007 ns |  1.00 |    0.00 |   0.0076 |        - |        - |      32 B |        1.00 |
|    BenchArrayInt |              Allocate |       1 |        45.933 ns |      0.6901 ns |      0.6117 ns |        45.977 ns |  4.58 |    0.06 |   0.0076 |        - |        - |      32 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized |       1 |        10.480 ns |      0.1064 ns |      0.0943 ns |        10.459 ns |  1.05 |    0.01 |   0.0076 |        - |        - |      32 B |        1.00 |
| BenchArrayString |                Create |       1 |         9.512 ns |      0.2020 ns |      0.1791 ns |         9.442 ns |  0.95 |    0.02 |   0.0076 |        - |        - |      32 B |        1.00 |
| BenchArrayString |              Allocate |       1 |        47.115 ns |      0.4613 ns |      0.4089 ns |        47.239 ns |  4.70 |    0.04 |   0.0076 |        - |        - |      32 B |        1.00 |
| BenchArrayString | AllocateUninitialized |       1 |         9.621 ns |      0.1906 ns |      0.3533 ns |         9.533 ns |  0.99 |    0.04 |   0.0076 |        - |        - |      32 B |        1.00 |
|                  |                       |         |                  |                |                |                  |       |         |          |          |          |           |             |
|    BenchArrayInt |                Create |       5 |        13.273 ns |      0.2988 ns |      0.3197 ns |        13.212 ns |  1.00 |    0.00 |   0.0115 |        - |        - |      48 B |        1.00 |
|    BenchArrayInt |              Allocate |       5 |        48.570 ns |      0.7197 ns |      0.6380 ns |        48.606 ns |  3.64 |    0.10 |   0.0114 |        - |        - |      48 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized |       5 |        13.704 ns |      0.1898 ns |      0.1776 ns |        13.657 ns |  1.03 |    0.02 |   0.0115 |        - |        - |      48 B |        1.00 |
| BenchArrayString |                Create |       5 |        15.731 ns |      0.1732 ns |      0.1353 ns |        15.740 ns |  1.17 |    0.03 |   0.0153 |        - |        - |      64 B |        1.33 |
| BenchArrayString |              Allocate |       5 |        53.037 ns |      0.3743 ns |      0.3318 ns |        53.119 ns |  3.97 |    0.10 |   0.0153 |        - |        - |      64 B |        1.33 |
| BenchArrayString | AllocateUninitialized |       5 |        15.644 ns |      0.1767 ns |      0.1653 ns |        15.627 ns |  1.17 |    0.03 |   0.0153 |        - |        - |      64 B |        1.33 |
|                  |                       |         |                  |                |                |                  |       |         |          |          |          |           |             |
|    BenchArrayInt |                Create |      10 |        15.196 ns |      0.1711 ns |      0.1429 ns |        15.183 ns |  1.00 |    0.00 |   0.0153 |        - |        - |      64 B |        1.00 |
|    BenchArrayInt |              Allocate |      10 |        51.142 ns |      0.4151 ns |      0.3467 ns |        51.112 ns |  3.37 |    0.04 |   0.0153 |        - |        - |      64 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized |      10 |        16.782 ns |      0.2404 ns |      0.2249 ns |        16.719 ns |  1.11 |    0.02 |   0.0153 |        - |        - |      64 B |        1.00 |
| BenchArrayString |                Create |      10 |        23.496 ns |      0.3257 ns |      0.2720 ns |        23.519 ns |  1.55 |    0.02 |   0.0249 |        - |        - |     104 B |        1.62 |
| BenchArrayString |              Allocate |      10 |        63.847 ns |      0.8255 ns |      0.6445 ns |        64.097 ns |  4.20 |    0.04 |   0.0248 |        - |        - |     104 B |        1.62 |
| BenchArrayString | AllocateUninitialized |      10 |        23.543 ns |      0.2681 ns |      0.2377 ns |        23.540 ns |  1.55 |    0.02 |   0.0249 |        - |        - |     104 B |        1.62 |
|                  |                       |         |                  |                |                |                  |       |         |          |          |          |           |             |
|    BenchArrayInt |                Create |     100 |        83.160 ns |      1.6437 ns |      1.7587 ns |        82.706 ns |  1.00 |    0.00 |   0.1013 |        - |        - |     424 B |        1.00 |
|    BenchArrayInt |              Allocate |     100 |       120.106 ns |      1.9915 ns |      1.8629 ns |       119.715 ns |  1.44 |    0.05 |   0.1013 |        - |        - |     424 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized |     100 |        84.808 ns |      0.6549 ns |      0.5113 ns |        84.780 ns |  1.01 |    0.02 |   0.1013 |        - |        - |     424 B |        1.00 |
| BenchArrayString |                Create |     100 |       160.460 ns |      1.8637 ns |      1.7433 ns |       160.166 ns |  1.93 |    0.05 |   0.1969 |        - |        - |     824 B |        1.94 |
| BenchArrayString |              Allocate |     100 |       196.155 ns |      2.7282 ns |      2.4185 ns |       195.607 ns |  2.35 |    0.04 |   0.1969 |        - |        - |     824 B |        1.94 |
| BenchArrayString | AllocateUninitialized |     100 |       159.295 ns |      2.0602 ns |      1.7203 ns |       159.233 ns |  1.91 |    0.04 |   0.1969 |        - |        - |     824 B |        1.94 |
|                  |                       |         |                  |                |                |                  |       |         |          |          |          |           |             |
|    BenchArrayInt |                Create |    1000 |       777.217 ns |     12.3406 ns |     10.9396 ns |       775.492 ns |  1.00 |    0.00 |   0.9613 |        - |        - |    4024 B |        1.00 |
|    BenchArrayInt |              Allocate |    1000 |       789.251 ns |     10.2752 ns |      9.6114 ns |       787.657 ns |  1.02 |    0.01 |   0.9613 |        - |        - |    4024 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized |    1000 |       187.433 ns |      3.7948 ns |      7.6657 ns |       184.953 ns |  0.25 |    0.01 |   0.9584 |        - |        - |    4024 B |        1.00 |
| BenchArrayString |                Create |    1000 |     1,554.818 ns |     15.1447 ns |     14.1664 ns |     1,559.290 ns |  2.00 |    0.04 |   1.9150 |        - |        - |    8024 B |        1.99 |
| BenchArrayString |              Allocate |    1000 |     1,562.925 ns |     13.4857 ns |     11.9547 ns |     1,563.005 ns |  2.01 |    0.03 |   1.9150 |        - |        - |    8024 B |        1.99 |
| BenchArrayString | AllocateUninitialized |    1000 |     1,562.850 ns |     19.5024 ns |     17.2883 ns |     1,564.989 ns |  2.01 |    0.03 |   1.9150 |        - |        - |    8024 B |        1.99 |
|                  |                       |         |                  |                |                |                  |       |         |          |          |          |           |             |
|    BenchArrayInt |                Create |   10000 |     7,200.916 ns |     67.9913 ns |     63.5991 ns |     7,185.784 ns |  1.00 |    0.00 |   9.5215 |        - |        - |   40024 B |        1.00 |
|    BenchArrayInt |              Allocate |   10000 |     7,149.810 ns |     84.2155 ns |     78.7753 ns |     7,130.345 ns |  0.99 |    0.02 |   9.5215 |        - |        - |   40024 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized |   10000 |       651.649 ns |      7.0545 ns |      5.8909 ns |       650.162 ns |  0.09 |    0.00 |   9.5234 |        - |        - |   40024 B |        1.00 |
| BenchArrayString |                Create |   10000 |    14,001.614 ns |    237.3965 ns |    198.2368 ns |    14,009.929 ns |  1.94 |    0.03 |  18.8599 |        - |        - |   80024 B |        2.00 |
| BenchArrayString |              Allocate |   10000 |    14,089.164 ns |    142.5671 ns |    119.0500 ns |    14,099.614 ns |  1.96 |    0.02 |  18.8599 |        - |        - |   80024 B |        2.00 |
| BenchArrayString | AllocateUninitialized |   10000 |    14,088.178 ns |     65.6579 ns |     58.2040 ns |    14,101.808 ns |  1.95 |    0.02 |  18.8599 |        - |        - |   80024 B |        2.00 |
|                  |                       |         |                  |                |                |                  |       |         |          |          |          |           |             |
|    BenchArrayInt |                Create |  100000 |    29,484.404 ns |    311.6528 ns |    382.7376 ns |    29,507.994 ns |  1.00 |    0.00 | 124.9695 | 124.9695 | 124.9695 |  400066 B |        1.00 |
|    BenchArrayInt |              Allocate |  100000 |    29,567.013 ns |    449.1566 ns |    420.1414 ns |    29,604.886 ns |  1.00 |    0.02 | 124.9695 | 124.9695 | 124.9695 |  400066 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized |  100000 |    26,492.538 ns |    292.9388 ns |    259.6827 ns |    26,564.154 ns |  0.90 |    0.02 | 124.9695 | 124.9695 | 124.9695 |  400066 B |        1.00 |
| BenchArrayString |                Create |  100000 |    54,173.601 ns |    672.7485 ns |    596.3741 ns |    54,227.924 ns |  1.84 |    0.04 | 249.9390 | 249.9390 | 249.9390 |  800107 B |        2.00 |
| BenchArrayString |              Allocate |  100000 |    54,339.854 ns |    967.7527 ns |    808.1174 ns |    54,227.414 ns |  1.85 |    0.03 | 249.9390 | 249.9390 | 249.9390 |  800107 B |        2.00 |
| BenchArrayString | AllocateUninitialized |  100000 |    53,836.765 ns |    441.4254 ns |    391.3121 ns |    53,730.786 ns |  1.83 |    0.03 | 249.9390 | 249.9390 | 249.9390 |  800107 B |        2.00 |
|                  |                       |         |                  |                |                |                  |       |         |          |          |          |           |             |
|    BenchArrayInt |                Create | 1000000 |   208,606.318 ns |  3,285.4765 ns |  3,073.2368 ns |   208,015.698 ns |  1.00 |    0.00 | 999.7559 | 999.7559 | 999.7559 | 4000356 B |        1.00 |
|    BenchArrayInt |              Allocate | 1000000 |   207,851.978 ns |  3,900.6592 ns |  3,830.9689 ns |   206,175.659 ns |  1.00 |    0.02 | 999.7559 | 999.7559 | 999.7559 | 4000357 B |        1.00 |
|    BenchArrayInt | AllocateUninitialized | 1000000 |   183,179.046 ns |  1,604.5172 ns |  1,500.8664 ns |   183,096.216 ns |  0.88 |    0.01 | 999.7559 | 999.7559 | 999.7559 | 4000329 B |        1.00 |
| BenchArrayString |                Create | 1000000 | 2,440,452.062 ns | 48,071.0001 ns | 64,173.4044 ns | 2,449,305.859 ns | 11.64 |    0.36 | 140.6250 | 140.6250 | 140.6250 | 8000068 B |        2.00 |
| BenchArrayString |              Allocate | 1000000 | 2,427,959.420 ns | 37,518.6029 ns | 31,329.7327 ns | 2,437,695.117 ns | 11.68 |    0.20 | 132.8125 | 132.8125 | 132.8125 | 8000066 B |        2.00 |
| BenchArrayString | AllocateUninitialized | 1000000 | 2,412,512.165 ns | 33,286.0854 ns | 29,507.2477 ns | 2,419,748.047 ns | 11.60 |    0.18 | 140.6250 | 140.6250 | 140.6250 | 8000068 B |        2.00 |

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, these benchmark shows that GC.Allocate has high overhead for small sizes, but they do not show where the new API is going to land exactly.

I would be interesting to benchmark new int[1] vs. Array.CreateInstanceFromArrayType(typeof(int[]), 1) vs. Array.CreateInstance(typeof(int), 1). We want Array.CreateInstanceFromArrayType performance to be as close as possible to new int[1].

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Thank you for your help @jkotas!

@@ -66,6 +61,7 @@ class ArrayNative

};

extern "C" void QCALLTYPE Array_CreateInstance(QCall::TypeHandle pTypeHnd, INT32 rank, INT32* pLengths, INT32* pBounds, BOOL createFromArrayType, QCall::ObjectHandleOnStack retArray);
Copy link
Member

Choose a reason for hiding this comment

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

Converting FCALL to QCALL makes the existing CreateInstance up to 10% faster.

@jkotas jkotas force-pushed the Array.CreateInstanceFromArrayType branch 2 times, most recently from 074abff to 19271ef Compare November 16, 2023 07:43
@jkotas jkotas force-pushed the Array.CreateInstanceFromArrayType branch from 7135a0c to 85a44ff Compare November 16, 2023 08:07
@jkotas
Copy link
Member

jkotas commented Nov 16, 2023

@adamsitnik This PR should be green now. Any other feedback?

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@jkotas LGTM, big thanks for your help!

@jkotas jkotas merged commit 115199d into dotnet:main Nov 16, 2023
175 of 178 checks passed
jkotas added a commit to jkotas/runtime that referenced this pull request Nov 17, 2023
The CoreCLR type loader allowed creating arrays of System.Void. Many operations with these invalid array types failed, often in inscrutable ways. For example, GetInterfaces() call failed with type load exception of IEnumerable type. The exact failure modes are different between runtimes.

It is better to disallow creating these invalid array types in the first place, across all runtimes, to make the behavior robust and consistent.

Related to dotnet#88620

Fixes dotnet#94086
jkotas added a commit that referenced this pull request Nov 17, 2023
The CoreCLR type loader allowed creating arrays of System.Void. Many operations with these invalid array types failed, often in inscrutable ways. For example, GetInterfaces() call failed with type load exception of IEnumerable type. The exact failure modes are different between runtimes.

It is better to disallow creating these invalid array types in the first place, across all runtimes, to make the behavior robust and consistent.

Related to #88620

Fixes #94086
@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants