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

Why is array still covariant? #41565

Closed
RamType0 opened this issue Aug 30, 2020 · 21 comments
Closed

Why is array still covariant? #41565

RamType0 opened this issue Aug 30, 2020 · 21 comments
Labels
area-System.Runtime question Answer questions and provide assistance, not an issue with source code or documentation.

Comments

@RamType0
Copy link

Covariant array make lower performance of almost any scenario.
It seems that .NET Core already has many larger breaking change than covariant array,why is it still maintained?

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner labels Aug 30, 2020
@ghost
Copy link

ghost commented Aug 30, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

@GrabYourPitchforks
Copy link
Member

This would be an absolutely massive breaking change that would affect libraries and applications all the way up and down the stack. Array writes are already highly optimized and the performance gains for most applications would be negligible, so this doesn't justify the break. For applications which really do need to eke out these extra few percentage points, you could always cast the T[] to a Span<T> manually, at which point writes do not incur covariance checks. (They'll still update the GC card table, though, which is where most of the overhead for standard array writes comes from anyway.)

@GrabYourPitchforks GrabYourPitchforks added area-System.Runtime question Answer questions and provide assistance, not an issue with source code or documentation. and removed area-Infrastructure-libraries labels Aug 30, 2020
@RamType0
Copy link
Author

This would be an absolutely massive breaking change that would affect libraries and applications all the way up and down the stack.

I didn't mean runtime internal massiveness.
If we do make array invariant,how external libraries (and also CoreFX part of runtime) are affected?
I think the breaking change for Process.Start was far much greater than this.

Array writes are already highly optimized and the performance gains for most applications would be negligible, so this doesn't justify the break.

#23393
The posted benchmark at here is seems to be representing massive performance improvement,isn't it?

For applications which really do need to eke out these extra few percentage points, you could always cast the T[] to a Span<T> manually, at which point writes do not incur covariance checks. (They'll still update the GC card table, though, which is where most of the overhead for standard array writes comes from anyway.)

Span<T> is not always faster than T[]( in platform which uses slow-span).

@svick
Copy link
Contributor

svick commented Aug 30, 2020

@RamType-0

I didn't mean runtime internal massiveness.

So what do you mean?

Span<T> is not always faster than T[]( in platform which uses slow-span).

If a platform is not updated to use fast span, what makes you think it will be updated to use invariant arrays?

If you're suggesting something that does not require updating the platform, then I think you need to be clearer about what that is.

@RamType0
Copy link
Author

@RamType-0

I didn't mean runtime internal massiveness.

So what do you mean?

I mean runtime compatibility.(e.g. Process.Start no longer accepts uri by default in .NET Core. I think that was huge breaking changes.)

Span<T> is not always faster than T[]( in platform which uses slow-span).

If a platform is not updated to use fast span, what makes you think it will be updated to use invariant arrays?

The solution Replacing T[] by Span'<T'> requires to be implemented by library provider,and would rather make lower performance in slow-span platform.

The solution treating array as invariant doesn't requires any update by library provider,and also never make lower performance of any other platform.

If you're suggesting something that does not require updating the platform, then I think you need to be clearer about what that is.

I'm pretty sure that
I'm suggesting something definitely requires updating the platform.

@huoyaoyuan
Copy link
Member

huoyaoyuan commented Aug 30, 2020

(e.g. Process.Start no longer accepts uri by default in .NET Core. I think that was huge breaking changes.)

Process.Start is a much more minor breaking change because:

  • Process is much less used than array. Array is one of the fundamental types.
  • The change in Process.Start is caused by a larger feature deprecation. It's very unlikely to be an implementation detail of something that's not influenced by the whole deprecation.

To be clear, I mean that any breaking change in fundamental type like array will be extremely huge.

@RamType0
Copy link
Author

To be clear, I mean that any breaking change in fundamental type like array will be extremely huge.

Have you ever wrote a code using array's covariantness?
What makes "any breaking change in fundamental type" huge is because in most of case, it's behaviour is widely used.

But I have not ever seen the code using the behaviour that "array is covariant".
(I have seen a code about array's covariantness only in internal Memory<T> or Span<T> implementation, but it was just a safety check.This code will not be broken.)

From the ancient of .NET Framework(after 2.0,which has generics), we know that array's covariantness is evil and waste,or most of us are just thinking array is invariant.

That's why I think make array invariant is a minor breaking change.

@RamType0
Copy link
Author

RamType0 commented Aug 30, 2020

@GrabYourPitchforks

I think it is inconsistent to reject this feature by the reason you said in this issue.

Array writes are already highly optimized and the performance gains for most applications would be negligible, so this doesn't justify the break. For applications which really do need to eke out these extra few percentage points, you could always cast the T[] to a Span<T> manually, at which point writes do not incur covariance checks. (They'll still update the GC card table, though, which is where most of the overhead for standard array writes comes from anyway.)

If you really think so,you should also reject #23393 , isn't it?

I think it is acceptable to reject this feature because of runtime internal implementation cost,or because of any other reliable reason.

@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Aug 30, 2020

@RamType-0, #23393 does not change the way arrays work; it instead introduces a new type with different behavior.

The solution Replacing T[] by Span'<T'> requires to be implemented by library provider,and would rather make lower performance in slow-span platform.

.NET is getting faster at each release, but some of these performance improvements will inevitably require code changes to be fully taken advantage of.

The slow span is not something we should be worried about; all frameworks implementing it (.NET Framework and .NET Core 1.X) are considered legacy (outright unsupported in .NET Core 1.X's case) and unsuitable for the development of new projects. And it's not that slow after all.

@RamType0
Copy link
Author

@RamType-0, #23393 does not change the way arrays work; it instead introduces a new type with different behavior.

I'm not comparing it with make array invariant.
I'm comparing it with the solution that make span from array.
’InvariantArray’ is a persistent buffer with better performance of writing reference types and not performant for value types, and requires copy when we convert it to array.

The solution make span from array is performant for any types, and doesn't require any overhead while convering it to array.

The slow span is not something we should be worried about; all frameworks implementing it (.NET Framework and .NET Core 1.X) are considered legacy (outright unsupported in .NET Core 1.X's case) and unsuitable for the development of new projects. And it's not that slow after all.

I think there are stiil
3 viable platforms.

  • .NET Core 3.1,.NET 5
    Of course they are viable!
    We could have amazing performance and nighty feature at here.

  • .NET Framework 4.8
    Built in to Windows 10.
    We don't need to be worry about it's installation.
    Viable when size of exe should be small.(Otherwise, we can use .NET Core self-contained build.)

  • Unity
    I think this is the most big issue at here.
    Performance sensitive scenario is very common in Unity.
    It's uses slow-span,and in the first place,library providers for Unity could hardly use any other library.
    Even unity official package it self usually has conflict of assembly. And they finally started to manual renaming of assembly verification.(e.g. System.Runtime.CompilerServices.Unsafe → Unity.Burst.Unsafe)

@RamType0
Copy link
Author

Oh,I was missing one ,UAP.
UAP still doesn't support .NET Standard2.1.

@huoyaoyuan
Copy link
Member

Have you ever wrote a code using array's covariantness?

No, but seen in BCL. In fact, the implementations of Span<T> and ImmutableArray<T> use it.

But, answering "no" for myself isn't enough for this question. If we do want to introduce "array invariant mode", the first step must be investigating a very large amount of libraries (both open and closed source).

@RamType0
Copy link
Author

No, but seen in BCL. In fact, the implementations of Span<T> and ImmutableArray<T> use it.

I know that of Span<T> and that was just a safety check(Ensures that passed array is T[]).
So it will not be broke.

@GrabYourPitchforks
Copy link
Member

@RamType-0 Can you clarify the particular question being asked here? So far I think we've addressed two questions.

  1. Does the framework offer types that can act as non-covariant arrays? Yes, see Span<T> and InvariantArray<T> (under discussion) for some examples.
  2. Will the framework be updated to make standard T[] arrays non-covariant? No, this would break too much code.

Did we correctly understand the question being asked?

@teo-tsirpanis
Copy link
Contributor

@RamType-0, could you also tell us what do you need invariant arrays for? What kind of performance-critical code are you working on, for which platform, and why is using a Span not acceptable?

Moreover, how much optimized is your code? There is a chance that the variant array check is not your bottleneck. Have you tried Span and found performance differences while benchmarking?

@ufcpp
Copy link
Contributor

ufcpp commented Sep 1, 2020

Have you ever wrote a code using array's covariantness?

Unity
I think this is the most big issue at here.

Yes, I have for performance in Unity.

@RamType0
Copy link
Author

RamType0 commented Sep 1, 2020

Have you ever wrote a code using array's covariantness?

Unity
I think this is the most big issue at here.

Yes, I have for performance in Unity.

Did You mean that you have wrote a code using array's covariantness,and that will not work if we make it invariant?
If so,would you tell me what was the code like?

Also, I'm proposing this feature for .NET Core,will not affect Unity.
If such a code only available in Unity, maybe that's not a big problem. Unity already has many its local "culture".
We often migrating from .NET Framework to .NET Core,but seldom migrating from Unity to .NET Core.
The code using array as invariant will work on runtime which array is covariant.

  1. Does the framework offer types that can act as non-covariant arrays? Yes, see Span<T> and InvariantArray<T> (under discussion) for some examples.

I'm not requiring such a type any more,but if you think Span<T> is a good existing solution,why is InvariantArray<T> also required?

2. Will the framework be updated to make standard T[] arrays non-covariant? No, this would break too much code.

I'm proposing make standard T[] as invariant through runtime invocation option(or by default).
I'm really not sure about concrete example of such a code. What is that code like?
And are they popular than Process.Start?

It was very hard to find broken Process.Start statically (it has no changes for its signature),but I think finding broken array's covariant usage would be found statically ,and easily.

@RamType0
Copy link
Author

RamType0 commented Sep 1, 2020

@RamType-0, could you also tell us what do you need invariant arrays for? What kind of performance-critical code are you working on, for which platform, and why is using a Span not acceptable?

Moreover, how much optimized is your code? There is a chance that the variant array check is not your bottleneck. Have you tried Span and found performance differences while benchmarking?

I got unexpectedly low performance for WithRef in this benchmark.

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.450 (2004/?/20H1)
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=5.0.100-preview.8.20417.9
  [Host]     : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT
  Job-UBSNUT : .NET Core 3.1.7 (CoreCLR 4.700.20.36602, CoreFX 4.700.20.37001), X64 RyuJIT

InvocationCount=1  UnrollFactor=1  
Method Categories Mean Error StdDev Median
WithoutRef WithoutRef,RawAction 29.84 ms 0.254 ms 0.225 ms 29.77 ms
WithoutRefNoBoundsCheck WithoutRef,RawAction 27.16 ms 0.181 ms 0.151 ms 27.20 ms
WithoutRefLocalListCount WithoutRef,RawAction 30.28 ms 0.172 ms 0.152 ms 30.28 ms
WithRef WithRef,RawAction 53.41 ms 0.264 ms 0.221 ms 53.34 ms
WithRefNoBoundsCheck WithRef,RawAction 53.23 ms 0.267 ms 0.250 ms 53.27 ms
WithRefLocalListCount WithRef,RawAction 53.40 ms 0.165 ms 0.138 ms 53.39 ms
WithoutRefWraped WithoutRef,StructWrapedAction 31.43 ms 0.593 ms 1.302 ms 30.86 ms
WithoutRefNoBoundsCheckWraped WithoutRef,StructWrapedAction 26.25 ms 0.298 ms 0.233 ms 26.24 ms
WithoutRefLocalListCountWraped WithoutRef,StructWrapedAction 27.61 ms 0.546 ms 0.629 ms 27.61 ms
WithRefWraped WithRef,StructWrapedAction 39.87 ms 0.156 ms 0.138 ms 39.90 ms
WithRefNoBoundsCheckWraped WithRef,StructWrapedAction 30.99 ms 0.119 ms 0.099 ms 30.97 ms
WithRefLocalListCountWraped WithRef,StructWrapedAction 34.37 ms 0.597 ms 0.498 ms 34.57 ms

Maybe this because of covariant array.
But I also think that because Action is sealed,we do not need covariant-check for this even if array is covariant.

@teo-tsirpanis
Copy link
Contributor

This benchmark seems fishy to me. On the first run, it calls a delegate from an array, and then it sets it to null, causing almost always an exception. The overhead raising and immediately catching an excpetion quite certainly dwarfs the overhead of arrays being covariant.

In other words, the provided benchmarks failed to demonstrate the performance impact caused by the arrays' covariance. Try chaning the benchmarks so that the delegates are not set to null but simply called. You could add Span to the comparison as well.


It also seems to me that you are a Unity developer and mostly concerned about Unity's performance. Unfortunately there is not much we can do, apart from waiting from them to add support for .NET Standard 2.1 which was said to take quite a lot time.

@RamType0
Copy link
Author

RamType0 commented Sep 1, 2020

This benchmark seems fishy to me. On the first run, it calls a delegate from an array, and then it sets it to null, causing almost always an exception. The overhead raising and immediately catching an excpetion quite certainly dwarfs the overhead of arrays being covariant.

In other words, the provided benchmarks failed to demonstrate the performance impact caused by the arrays' covariance. Try chaning the benchmarks so that the delegates are not set to null but simply called. You could add Span to the comparison as well.

Can't you see [IterationsSetup]?
Is doesnt raise null reference exception.(It raises in my old commit,but I posted here a new benchmark from the beginning)

Using external dll from package often resulted in assembly confliction in Unity.
We could see unity official package often causes assembly confliction of System.Runtime.CompilerServices.Unsafe.
That's why we could hardly use span in library which also supports unity.

@teo-tsirpanis
Copy link
Contributor

Oh, yeah, I just realized the array was reset after each iteration.

@GrabYourPitchforks GrabYourPitchforks removed the untriaged New issue has not been triaged by the area owner label Sep 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
Development

No branches or pull requests

7 participants