Go through ImmutableArray and switch manual looping to Array.Copy #32373

Therzok · 2020-02-15T15:10:10Z

Make the array copying consistent and use the optimized Array.Copy API in each case.

Remove the only 'CreateDefensiveCopy' call and the method, and just forward to another overload

Passing explicit parameters should avoid calling GetLowerBound

jkotas · 2020-02-15T16:15:26Z

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs

-                array[i] = items[start + i];
-            }
-
+            Array.Copy(items, start, array, 0, length);


What is the break-even point for this? This will regress small sizes.

I'll setup a benchmark for this.

Answered in a comment below

So it makes some cases slower and some faster. I think we would need to additional data to tell whether this is an improvement on average. E.g. we would need data about how this method is used by Roslyn that is a heavy user of ImmutableArrays to make sure that this won't regress Roslyn performance.

The rest of the PR looks good to me. Could you please revert this change so that we can take the rest?

I can definitely split these across multiple atomic PRs so we tackle each different problem individually

jkotas · 2020-02-15T16:17:02Z

Note that 3-argument version of Array.Copy is more efficient than the versions with more arguments in current master. The GetLowerBound problem does not exist in .NET Core after dotnet/coreclr#27634

System.Collections.Immutable runs on older runtimes too. So it is a interesting question whether to optimize it for latest .NET Core or for old runtimes. Ifdefs maybe the best answer.

Therzok · 2020-02-15T16:40:54Z

Interesting. Well, I saw the extra call happening in code and most of the other code was using the 5 argument version. Also, there's a mention of this kind of optimization here, at the Peanut Butter section.

Most notably for this PR is consistency. Even List<T> uses Array.Copy, even in its CopyTo implementation.

I imagine if List<T> uses this mechanism everywhere, it should be okay to use it everywhere in ImmutableArray. Regardless, I'll setup a benchmark to see which array sizes this will regress performance on.

jkotas · 2020-02-15T16:59:54Z

there's a mention of this kind of optimization here

That was the best way for .NET Core 3. It is no longer the best way for .NET 5.

In current master, we should be using the fewer-argument overload of Array.Copy where possible after dotnet/coreclr#27641 .

Therzok · 2020-02-15T17:42:16Z

I've setup some benchmarks for the behaviour. Int32 and Object results.

Therzok · 2020-02-15T17:43:17Z

Not sure how to test with .NET 5 though. It seems there's a small regression for small arrays of int, Count < 8, but on the object scenario it's an improvement.

Therzok · 2020-02-15T17:55:07Z

Added another result for small int32 arrays. Cut-off seems to be at Count == 16.

Therzok · 2020-02-15T18:04:37Z

Now I've just made myself curious of perf characteristics of:

var arr = new T[Count];
var src = items.AsSpan(start, length);
var dest = new Span<T>(arr, offset);
src.CopyTo(dest);

…on, which is more optimized on .NET5

Therzok · 2020-02-15T18:14:14Z

Hah, would you look at that:

BenchmarkDotNet=v0.12.0, OS=macOS 10.15.3 (19D76) [Darwin 19.3.0]
Intel Core i7-4980HQ CPU 2.80GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.101
  [Host]     : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT
  DefaultJob : .NET Core 3.1.1 (CoreCLR 4.700.19.60701, CoreFX 4.700.19.60801), X64 RyuJIT

Method	Count	Mean	Error	StdDev	Ratio	RatioSD
Loop	1	5.039 ns	0.0326 ns	0.0305 ns	1.00	0.00
Copy3Arg	1	21.679 ns	0.0681 ns	0.0637 ns	4.30	0.03
Copy5Arg	1	15.476 ns	0.0810 ns	0.0632 ns	3.07	0.02
Span	1	8.150 ns	0.0546 ns	0.0484 ns	1.62	0.01

Loop	16	16.076 ns	0.0962 ns	0.0853 ns	1.00	0.00
Copy3Arg	16	23.715 ns	0.1901 ns	0.1778 ns	1.48	0.01
Copy5Arg	16	18.222 ns	0.0824 ns	0.0771 ns	1.13	0.01
Span	16	11.623 ns	0.1140 ns	0.1010 ns	0.72	0.01

Loop	32	30.684 ns	0.1605 ns	0.1423 ns	1.00	0.00
Copy3Arg	32	28.539 ns	0.1637 ns	0.1451 ns	0.93	0.00
Copy5Arg	32	22.666 ns	0.1230 ns	0.1151 ns	0.74	0.00
Span	32	16.398 ns	0.1793 ns	0.1677 ns	0.53	0.01

jkotas · 2020-02-15T22:08:47Z

System/Collections/Immutable/ImmutableArray.cs(157,31): error CS0103: The name 'length' does not exist in the current context

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs

Therzok · 2020-02-15T22:34:22Z

Sorry for that!

jkotas · 2020-02-15T22:57:07Z

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs

-            if (items == null)
+            if (items == null || items.Length == 0)
            {
                return Create<T>();


Can this just return ImmutableArray<T>.Empty?

Yeah, I was thinking about that in the initial variant. I went for consistency, but I can change this, and if needed, all the others

I think it would be a good idea to avoid the indirection via Create method.

Therzok · 2020-02-16T01:11:19Z

Superseded by #32386 and #32387

Therzok added 3 commits February 15, 2020 08:39

Change some loop copying to Array.Copy

c2536bf

Refactor ImmutableArray.Create<T>(params T[])

f25e34a

Remove the only 'CreateDefensiveCopy' call and the method, and just forward to another overload

Optimize Array.Copy calls in ImmutableArray

4ea2c32

Passing explicit parameters should avoid calling GetLowerBound

Dotnet-GitSync-Bot added the area-System.Collections label Feb 15, 2020

jkotas reviewed Feb 15, 2020

View reviewed changes

Therzok added 2 commits February 15, 2020 20:05

Switch back the possible Array.Copy calls to use the 3 argument versi…

5f3b531

…on, which is more optimized on .NET5

Use 3 argument version here

5a47f52

More Array.Copy changes

16865b0

Therzok commented Feb 15, 2020

View reviewed changes

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs Show resolved Hide resolved

Fix build

33ffb6a

jkotas reviewed Feb 15, 2020

View reviewed changes

Therzok closed this Feb 16, 2020

Therzok deleted the immutable-array-copy branch February 16, 2020 08:36

This was referenced Jun 20, 2020

Inconsistent null checks in Array.Copy #27888

Closed

Perf: String.replicate from O(n) to O(log(n)), up to 12x speed improvement dotnet/fsharp#9512

Merged

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Go through ImmutableArray and switch manual looping to Array.Copy #32373

Go through ImmutableArray and switch manual looping to Array.Copy #32373

Uh oh!

Conversation

Therzok commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Feb 15, 2020

Uh oh!

Therzok commented Feb 15, 2020

Uh oh!

jkotas commented Feb 15, 2020

Uh oh!

Therzok commented Feb 15, 2020

Uh oh!

Therzok commented Feb 15, 2020

Uh oh!

Therzok commented Feb 15, 2020

Uh oh!

Therzok commented Feb 15, 2020

Uh oh!

Therzok commented Feb 15, 2020

Uh oh!

jkotas commented Feb 15, 2020

Uh oh!

Uh oh!

Therzok commented Feb 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Therzok commented Feb 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Therzok commented Feb 15, 2020 •

edited

Loading