Add Array.GetMaxLength<T>

[huoyaoyuan]

Closes #31366

Not encountering the complexity of dividing with object size under 32bit, because that limit shouldn't be meaningful in real world.

Replaces usages of the magic constant with the new api.

[Dotnet-GitSync-Bot]

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.

[huoyaoyuan]

Is Unsafe.SizeOf<T> a reliable pattern that JIT can reduce to a constant?
Sharplab doesn't tell result for generics.

[jeffhandley]

@stephentoub or @jkotas -- It's unclear to me what the next steps for this PR should be; can you help?

[jkotas]

I think we should:

• Unify the max limit for all types
• Change the API to be non-generic - just return a constant

[stephentoub]

Change the API to be non-generic - just return a constant

At that point, it also needn't be a method, so presumably just:

public static int MaxLength => 0x7FFFFFC7;

[tannergooding]

Is the max limit going to be potentially confusing for types where it is larger than the actual max limit?

For example, on a 32-bit computer the max limit for byte and ushort is 0x7FFFFFC7 but the max limit for int is just shy of 0x3FFFFFFF as anything else would be more than the addressable memory limit.
The intent, AFAICT, is to help handle the simple doubling cases by giving as close to the actual limit as possible which as a constant really only works for 64-bit apps/libraries and because the length is itself limited to 32-bits.

Likewise, will a constant and non-generic T be sufficient for other runtimes, such as say Unity, where they may want or need more type specific behavior?

[jkotas]

For example, on a 32-bit computer the max limit for byte and ushort is 0x7FFFFFC7 but the max limit for int is just shy of 0x3FFFFFFF as anything else would be more than the addressable memory limit.

The practical limit on 32-bit is always a lot smaller due to address space fragmentation.

[stephentoub]

@jkotas, I pushed an update. Can you confirm this is what you had in mind?

[jkotas]

Yes, this looks good. Thanks!

[BruceForstall]

Could we apply this same maximum length limit (0X7FFFFFC7) to Span/ReadOnlySpan as well as arrays? This might allow the JIT to remove bounds checks in cases where currently we need to assume an iteration variable might overflow, e.g.,

private static int Problem(Span<int> a)
{
    int sum = 0;
    for (int i = 0; i < a.Length; i += 2) // 'i' will overflow for "a.Length == int.MaxValue - 1".
    {
        sum += a[i];
    }
    return sum;
}

But if we know that a.Length can't exceed 0X7FFFFFC7 then i += n can have n up to about 56 without worrying about overflow.

@stephentoub @jkotas

[jkotas]

It would require a breaking change. We would need to start validating the length and throwing an exception in Span constructors that take unmanaged pointers. Also, the affected Span constructors would become slower because of that.

Do you have any data on how often this optimization can kick in if we were to limit the max span length?

My gut feeling is that this is not worth. The extra instructions to check the Span length in constructors would cost more what we would save in these loops in typical programs. In addition to that, there is the cost of the breaking change.

[tannergooding]

For 64-bit wouldn't it be better to just retype the loop to be over nint internally? We can then know that it will never overflow and all usages in the loop can silently use the lower 32-bits without needing to downcast.

[jkotas]

For 64-bit wouldn't it be better to just retype the loop to be over nint internally?

Simple retying would change behavior. As written, the loop will crash for a.Length == Int32.MaxValue.

We would need to clone the loop and use the retyped clone only when we can check using precondition that the loop does not hit the boundary conditions.

[BruceForstall]

It would require a breaking change.

The only break (I believe) would be in the Span constructor taking a native pointer, which would fail if given a size >= 0X7FFFFFC7. (It's always been limited to be <= 0x7FFFFFFF.) So it's only a very small set of values that would be breaking. Yes, these constructors would need a check and an (uncommon) throw so might impact inlining.

Do you have any data on how often this optimization can kick in if we were to limit the max span length?

I have a prototype implementation that shows somewhat minimal diffs, across all our SuperPMI collections, but the problem with the implementation, and one of my main motivations for asking the question, is that the JIT likes to treat Span and array bounds checks the same, so I'm worried there are places where we're not optimizing arrays as well as we could because we need to "pessimize" our assumptions to consider that Span has a different maximum length. Possibly the JIT could do a better job distinguishing Span and array bounds checks and that problem would go away, but since Span is so much like an array it would be nice to treat them equivalently here, also.

[Edit] I found an implementation in the JIT that I think isolates just the missing optimizations from the Span max length being INT_MAX instead of the same as array, and it's pretty small. Here are the diffs.

Asm Diffs

libraries.crossgen2.windows.x64.checked.mch:

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 34369884 (overridden on cmd)
Total bytes of diff: 34369789 (overridden on cmd)
Total bytes of delta: -95 (-0.00 % of base)
    diff is an improvement.
    relative diff is an improvement.

Detail diffs

Top file improvements (bytes):
         -35 : 44792.dasm (-6.51% of base)
         -32 : 44793.dasm (-5.39% of base)
          -8 : 28480.dasm (-0.32% of base)
          -8 : 38462.dasm (-0.48% of base)
          -8 : 39447.dasm (-1.31% of base)
          -4 : 47062.dasm (-2.44% of base)

6 total files with Code Size differences (6 improved, 0 regressed), 0 unchanged.

Top method improvements (bytes):
         -35 (-6.51% of base) : 44792.dasm - System.DateTimeFormat:IsValidCustomTimeFormat(System.ReadOnlySpan`1[System.Char],bool):bool
         -32 (-5.39% of base) : 44793.dasm - System.DateTimeFormat:IsValidCustomDateFormat(System.ReadOnlySpan`1[System.Char],bool):bool
          -8 (-0.48% of base) : 38462.dasm - FormatLiterals:Init(System.ReadOnlySpan`1[System.Char],bool):this
          -8 (-0.32% of base) : 28480.dasm - System.Diagnostics.Tracing.EventPipePayloadDecoder:DecodePayload(byref,System.ReadOnlySpan`1[System.Byte]):System.Object[]
          -8 (-1.31% of base) : 39447.dasm - System.Globalization.CultureData:ConvertIcuTimeFormatString(System.ReadOnlySpan`1[System.Char]):System.String
          -4 (-2.44% of base) : 47062.dasm - System.RuntimeTypeHandle:GetTypeHelper(System.Type,System.Type[],System.ReadOnlySpan`1[System.Int32]):System.Type

Top method improvements (percentages):
         -35 (-6.51% of base) : 44792.dasm - System.DateTimeFormat:IsValidCustomTimeFormat(System.ReadOnlySpan`1[System.Char],bool):bool
         -32 (-5.39% of base) : 44793.dasm - System.DateTimeFormat:IsValidCustomDateFormat(System.ReadOnlySpan`1[System.Char],bool):bool
          -4 (-2.44% of base) : 47062.dasm - System.RuntimeTypeHandle:GetTypeHelper(System.Type,System.Type[],System.ReadOnlySpan`1[System.Int32]):System.Type
          -8 (-1.31% of base) : 39447.dasm - System.Globalization.CultureData:ConvertIcuTimeFormatString(System.ReadOnlySpan`1[System.Char]):System.String
          -8 (-0.48% of base) : 38462.dasm - FormatLiterals:Init(System.ReadOnlySpan`1[System.Char],bool):this
          -8 (-0.32% of base) : 28480.dasm - System.Diagnostics.Tracing.EventPipePayloadDecoder:DecodePayload(byref,System.ReadOnlySpan`1[System.Byte]):System.Object[]

6 total methods with Code Size differences (6 improved, 0 regressed), 0 unchanged.

---

libraries.pmi.windows.x64.checked.mch:

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 50439195 (overridden on cmd)
Total bytes of diff: 50439168 (overridden on cmd)
Total bytes of delta: -27 (-0.00 % of base)
    diff is an improvement.
    relative diff is an improvement.

Detail diffs

Top file improvements (bytes):
         -19 : 135819.dasm (-14.96% of base)
          -8 : 194382.dasm (-1.95% of base)

2 total files with Code Size differences (2 improved, 0 regressed), 0 unchanged.

Top method improvements (bytes):
         -19 (-14.96% of base) : 135819.dasm - System.Xml.XmlTextReaderImpl:AdjustLineInfo(System.ReadOnlySpan`1[Char],bool,byref)
          -8 (-1.95% of base) : 194382.dasm - System.Formats.Asn1.BMPEncoding:GetChars(System.ReadOnlySpan`1[Byte],System.Span`1[Char],bool):int:this

Top method improvements (percentages):
         -19 (-14.96% of base) : 135819.dasm - System.Xml.XmlTextReaderImpl:AdjustLineInfo(System.ReadOnlySpan`1[Char],bool,byref)
          -8 (-1.95% of base) : 194382.dasm - System.Formats.Asn1.BMPEncoding:GetChars(System.ReadOnlySpan`1[Byte],System.Span`1[Char],bool):int:this

2 total methods with Code Size differences (2 improved, 0 regressed), 0 unchanged.

---

[jkotas]

The only break (I believe) would be in the Span constructor taking a native pointer

MemoryMarshal.CreateSpan and MemoryMarshal.CreateReadOnlySpan would need the check too.

[tannergooding]

Simple retying would change behavior. As written, the loop will crash for a.Length == Int32.MaxValue.

On 32-bit yes.

On 64-bit, since the actual indexing done behind the scenes needs to be nint, then we'd have i become 0x7FFF_FFFF or 0x8000_0000 both of which are greater than or equal to any a.Length (and less than nint.MaxValue which is 0x7FFF_FFFF_FFFF_FFFF).

Likewise since we know that i starts at 0 and the increment logic is purely positive we could just silently make these unsigned comparisons which would also solve the issue, including on 32-bit.

[tannergooding]

I think we have a few options given that we know valid indices must be positive and the length can never be greater than int.MaxValue. We have more options on 64-bit platforms where the backing index operations need to be 64-bit anyways and therefore nint is the technically correct type (and indeed we have to insert zero/sign-extensions for valid indexing today anyways, so hoisting that extension earlier seems like general goodness).