Make mutable generic collection interfaces implement read-only collection interfaces

[TylerBrinkley]

Closes #31001

[ghost]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Closes #31001

Author:	TylerBrinkley
Assignees:	-
Labels:	area-System.Collections
Milestone:	-

[teo-tsirpanis]

You also need to update the reference sources at https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/ref/System.Runtime.cs.

[TylerBrinkley]

You also need to update the reference sources at https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/ref/System.Runtime.cs.

Thanks, done. I assumed they were auto-generated given how they looked with fully qualified names.

[huoyaoyuan]

The assert is in GetActualImplementationForArrayGenericIListOrIReadOnlyListMethod in src\coreclr\vm\array.cpp. Interface implementation of arrays is specially handled by runtime.

[eiriktsarpalis]

cc @mangod9 @jkotas

[TylerBrinkley]

I'm sorry, I'm not well versed in c++. From what I gather from that method we use the inheritance depth to determine the starting method but with this change the inheritance depth of ICollection<T> and IReadOnlyList<T> are the same meaning we need some other way to differentiate between the two in order to pick the correct starting method. Also do I need to re-arrange the DIM methods to be last to maintain the current interface member order?

[jkotas]

Yes, the inheritance depth optimization is not going work. To get unblocked, you can try deleting it and call MemberLoader::FindMethodByName instead (it is the method call used by the assert). Then look into optimizing the perf once we achieve functional correctness.

[stephentoub]

@eiriktsarpalis, if we end up keeping this inheritance, that will tip the scales for special-casing IReadOnlyXx in LINQ: in particular for places where we only need the source's count, we'll want to look for IReadOnlyCollection rather than ICollection, since every occurrence of the latter will also be the former.

[TylerBrinkley]

I believe it was decided to not make any changes in .NET 9 that would rely on this inheritance hierarchy change in case it needs to be reverted late in the cycle.

[MichalStrehovsky]

Note that going to the default implemented method (that in turn calls another interface method) will involve a double interface dispatch. It remains to be seen how well (if at all) dynamic pgo will be able to get rid of it. The callsite in the default implementation is going to be megamorphic.

[aromaa]

Note that going to the default implemented method (that in turn calls another interface method) will involve a double interface dispatch.

Could the runtime optimize this to make the IReadOnlyCollection<T>.Count slot point directly to the ICollection<T>.Count? That way we could avoid the double interface dispatch.

[KennethHoff]

Isn't this also something PGO can help with?

[MichalStrehovsky]

The way PGO works is that it will collect all the possible types of this it saw at the callsite and then generate a cascade of if (thisType.GetType() == typeof(Foo)) Foo.Method();. There's a limit on how many checks can be generated that way, and each check has a cost. The more types you have that implement the ICollection but not IReadOnlyCollection, the worse it gets.

You'll typically not see this in microbenchmarks because those typically repeat the same thing over and over and dynamic PGO is really good at optimizing things that are repeated the same way over and over.

[tannergooding]

The double dispatch is largely a point in time problem.

None of the built-in BCL types will actually hit it, because we provide an actual implementation. Downstream implementers are either already implementing both ICollection<T> and IReadOnlyCollection<T> or can be pointed towards doing so with an analyzer so they don't incur the double dispatch either.

[Joe4evr]

I believe it was decided to not make any changes in .NET 9 that would rely on this inheritance hierarchy change in case it needs to be reverted late in the cycle.

But it could/would be done for .NET 10 (provided this change stays)?

[stephentoub]

I believe it was decided to not make any changes in .NET 9 that would rely on this inheritance hierarchy change in case it needs to be reverted late in the cycle.

But it could/would be done for .NET 10 (provided this change stays)?

I don't agree with the stance and plan to submit a PR once this goes in. If we're confident enough to make this change at this point, we have to be fine with subsequent changes being based on it, as they absolutely will be whether intentionally or not. And it's no worse reverting one large PR and one small PR than one large PR if we decide we must.

[TylerBrinkley]

I'm a bit out of my depth when it comes to these .NET Native AOT errors. Can anyone point me in the right direction here?

[MichalStrehovsky]

I'm a bit out of my depth when it comes to these .NET Native AOT errors. Can anyone point me in the right direction here?

I can help with that later but there's plenty of not-nativeaot failures to look at (the details already expired from the ci).

[eiriktsarpalis]

I've merged the latest main to trigger a fresh build, hopefully we can take a closer look at the failures there.

[KennethHoff]

These benchmark links lead to 401 pages; would it be possible to make them publicly available? (.. Or give me access? 🫠)

I'm intrigued by what's in it

[sebastienros]

@KennethHoff the fact that you have to use the link is a bug (we need to update some credentials), the results should appear automatically on this PR. I will paste them manually. The link is useful for MS devs to investigate these issues.

[sebastienros]

No significant improvement or regression on these benchmarks.

Json

| application             | json.base                 | json.pr                   |        |
| ----------------------- | ------------------------- | ------------------------- | ------ |
| Max CPU Usage (%)       |                        84 |                        79 | -5.95% |
| Max Cores usage (%)     |                     2,345 |                     2,220 | -5.33% |
| Max Working Set (MB)    |                        61 |                        62 | +1.64% |
| Max Private Memory (MB) |                        83 |                        84 | +1.20% |
| Build Time (ms)         |                     3,681 |                     4,008 | +8.88% |
| Start Time (ms)         |                       372 |                       375 | +0.81% |
| Published Size (KB)     |                   105,141 |                   105,141 |  0.00% |
| Symbols Size (KB)       |                        54 |                        54 |  0.00% |
| .NET Core SDK Version   | 9.0.100-preview.4.24222.1 | 9.0.100-preview.4.24222.1 |        |


| load                    | json.base  | json.pr    |         |
| ----------------------- | ---------- | ---------- | ------- |
| Max CPU Usage (%)       |         75 |         74 |  -1.33% |
| Max Cores usage (%)     |      2,108 |      2,073 |  -1.66% |
| Max Working Set (MB)    |         45 |         46 |  +2.22% |
| Max Private Memory (MB) |        363 |        363 |   0.00% |
| Build Time (ms)         |      3,628 |      3,302 |  -8.99% |
| Start Time (ms)         |          0 |          0 |         |
| Published Size (KB)     |     72,257 |     72,257 |   0.00% |
| Symbols Size (KB)       |          0 |          0 |         |
| .NET Core SDK Version   |    8.0.204 |    8.0.204 |         |
| First Request (ms)      |        130 |        131 |  +0.77% |
| Requests/sec            |  1,096,001 |  1,079,449 |  -1.51% |
| Requests                | 16,547,548 | 16,299,307 |  -1.50% |
| Mean latency (ms)       |       4.91 |       4.56 |  -7.13% |
| Max latency (ms)        |     327.14 |     203.38 | -37.83% |
| Bad responses           |          0 |          0 |         |
| Socket errors           |          0 |          0 |         |
| Read throughput (MB/s)  |     152.60 |     150.30 |  -1.51% |
| Latency 50th (ms)       |       0.30 |       0.30 |  +1.00% |
| Latency 75th (ms)       |       1.26 |       1.35 |  +7.14% |
| Latency 90th (ms)       |      14.55 |      14.23 |  -2.20% |
| Latency 99th (ms)       |      68.07 |      61.93 |  -9.02% |

Plaintext

| application             | plaintext.base            | plaintext.pr              |        |
| ----------------------- | ------------------------- | ------------------------- | ------ |
| Max CPU Usage (%)       |                        70 |                        71 | +1.43% |
| Max Cores usage (%)     |                     1,972 |                     1,993 | +1.06% |
| Max Working Set (MB)    |                        71 |                        72 | +1.41% |
| Max Private Memory (MB) |                        94 |                        94 |  0.00% |
| Build Time (ms)         |                     8,439 |                     7,690 | -8.88% |
| Start Time (ms)         |                       371 |                       373 | +0.54% |
| Published Size (KB)     |                   105,141 |                   105,141 |  0.00% |
| Symbols Size (KB)       |                        54 |                        54 |  0.00% |
| .NET Core SDK Version   | 9.0.100-preview.4.24222.1 | 9.0.100-preview.4.24222.1 |        |


| load                    | plaintext.base | plaintext.pr |          |
| ----------------------- | -------------- | ------------ | -------- |
| Max CPU Usage (%)       |             80 |           79 |   -1.25% |
| Max Cores usage (%)     |          2,228 |        2,210 |   -0.81% |
| Max Working Set (MB)    |             46 |           46 |    0.00% |
| Max Private Memory (MB) |            370 |          370 |    0.00% |
| Build Time (ms)         |          7,275 |        5,523 |  -24.08% |
| Start Time (ms)         |              0 |            0 |          |
| Published Size (KB)     |         72,257 |       72,257 |    0.00% |
| Symbols Size (KB)       |              0 |            0 |          |
| .NET Core SDK Version   |        8.0.204 |      8.0.204 |          |
| First Request (ms)      |             96 |           93 |   -3.12% |
| Requests/sec            |      9,349,725 |    9,480,593 |   +1.40% |
| Requests                |    141,159,099 |  143,145,312 |   +1.41% |
| Mean latency (ms)       |          14.02 |        35.72 | +154.78% |
| Max latency (ms)        |         519.44 |       997.23 |  +91.98% |
| Bad responses           |              0 |            0 |          |
| Socket errors           |              0 |            0 |          |
| Read throughput (MB/s)  |       1,126.40 |     1,136.64 |   +0.91% |
| Latency 50th (ms)       |           0.96 |         0.96 |    0.00% |
| Latency 75th (ms)       |           7.03 |        28.75 | +308.96% |
| Latency 90th (ms)       |          43.03 |       118.16 | +174.60% |
| Latency 99th (ms)       |         197.73 |       397.63 | +101.10% |

Fortunes

| application             | fortunes.base             | fortunes.pr               |        |
| ----------------------- | ------------------------- | ------------------------- | ------ |
| Max CPU Usage (%)       |                        84 |                        89 | +5.95% |
| Max Cores usage (%)     |                     2,343 |                     2,481 | +5.89% |
| Max Working Set (MB)    |                       556 |                       551 | -0.90% |
| Max Private Memory (MB) |                       584 |                       577 | -1.20% |
| Build Time (ms)         |                     4,182 |                     4,200 | +0.43% |
| Start Time (ms)         |                     2,241 |                     2,139 | -4.55% |
| Published Size (KB)     |                   105,148 |                   105,148 |  0.00% |
| Symbols Size (KB)       |                        55 |                        55 |  0.00% |
| .NET Core SDK Version   | 9.0.100-preview.4.24222.1 | 9.0.100-preview.4.24222.1 |        |


| load                    | fortunes.base | fortunes.pr |         |
| ----------------------- | ------------- | ----------- | ------- |
| Max CPU Usage (%)       |            38 |          38 |   0.00% |
| Max Cores usage (%)     |         1,058 |       1,073 |  +1.42% |
| Max Working Set (MB)    |            46 |          46 |   0.00% |
| Max Private Memory (MB) |           363 |         363 |   0.00% |
| Build Time (ms)         |         3,503 |       3,519 |  +0.46% |
| Start Time (ms)         |             0 |           0 |         |
| Published Size (KB)     |        72,257 |      72,257 |   0.00% |
| Symbols Size (KB)       |             0 |           0 |         |
| .NET Core SDK Version   |       8.0.204 |     8.0.204 |         |
| First Request (ms)      |           107 |         111 |  +3.74% |
| Requests/sec            |       439,629 |     441,483 |  +0.42% |
| Requests                |     6,638,326 |   6,666,291 |  +0.42% |
| Mean latency (ms)       |          1.38 |        1.32 |  -4.35% |
| Max latency (ms)        |         37.21 |       36.29 |  -2.47% |
| Bad responses           |             0 |           0 |         |
| Socket errors           |             0 |           0 |         |
| Read throughput (MB/s)  |        578.16 |      580.60 |  +0.42% |
| Latency 50th (ms)       |          1.08 |        1.09 |  +0.93% |
| Latency 75th (ms)       |          1.33 |        1.33 |   0.00% |
| Latency 90th (ms)       |          1.74 |        1.70 |  -2.30% |
| Latency 99th (ms)       |          9.91 |        7.99 | -19.37% |

[dotnet-policy-service]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[eiriktsarpalis]

Our current plan is to get the PR merged today by EOD today, in time for the change to make it for Preview 4.

[carlossanlop]

We need this change included in the Preview4 snap and the merge-on-green restriction is blocking us because a couple of failures are not getting linked to KnownBuildError issues as expected. @tannergooding has confirmed the failures are all unrelated, so I will bypass the requirements and merge it.

cc @JulieLeeMSFT