Allow reference types for pinned GC.AllocateArray() #89293

jthorborg · 2023-07-21T11:07:54Z

After an offline discussion, this is the "... or similar" part of the suggested feature set in this issue: #48703

This just relaxes front-end constraints as agreed for pinned array allocations with ref types, which is already possible and utilized internally.

The use case shortly is forming unmanaged references to array indices, where the array has a very long lifetime.

I added some extra coverage of the API, noticing the original constraint wasn't tested in the System.Runtime tests. I'm not sure if further documentation edits are necessary, or if it's fine to just remove the <remarks> section.

It's now possible to allocate pinned and default-initialized arrays that contain references. Fix #48703

Added a test that pins a reference type array and resolves references to indices with pointer arithmetic, checking that modifications are visible back in the original array. Test #48703

jthorborg · 2023-07-21T11:09:56Z

@dotnet-policy-service agree company="Unity Technologies"

jkotas · 2023-07-21T11:13:51Z

The same fix should be also applied to:
src\coreclr\nativeaot\System.Private.CoreLib\src\System\GC.NativeAot.cs
src\mono\System.Private.CoreLib\src\System\GC.Mono.cs

ghost · 2023-07-21T11:14:02Z

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

Issue Details

After an offline discussion, this is the "... or similar" part of the suggested feature set in this issue: #48703

This just relaxes front-end constraints as agreed for pinned array allocations with ref types, which is already possible and utilized internally.

The use case shortly is forming unmanaged references to array indices, where the array has a very long lifetime.

I added some extra coverage of the API, noticing the original constraint wasn't tested in the System.Runtime tests. I'm not sure if further documentation edits are necessary, or if it's fine to just remove the <remarks> section.

Author:	jthorborg
Assignees:	-
Labels:	`area-GC-coreclr`, `community-contribution`
Milestone:	-

jthorborg · 2023-07-21T11:48:53Z

The same fix should be also applied to: src\coreclr\nativeaot\System.Private.CoreLib\src\System\GC.NativeAot.cs src\mono\System.Private.CoreLib\src\System\GC.Mono.cs

Done, thanks I missed that. Are there tests to update for these as well?

MichalPetryka · 2023-07-21T12:03:53Z

I think that AllocateUninitializedArray should also be relaxed to allow this since it already accepts reference types.

jthorborg · 2023-07-21T12:20:04Z

I considered that but I worried that garbage bits on reference fields would perhaps not be good news for the GC.

EgorBo · 2023-07-21T14:04:03Z

Seems like there is a test that expects that exception - AllocateArrayUninitializedPinned_RefType_ThrowsArgumentException

I think that AllocateUninitializedArray should also be relaxed to allow this since it already accepts reference types.

Sounds like a very bad idea to me (unless we want it to work but it will never return garbage)

MichalPetryka · 2023-07-21T14:18:21Z

(unless we want it to work but it will never return garbage)

That's how it behaves today:

runtime/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

Lines 741 to 746 in 7982376

    
           if (!pinned) 
        
           { 
        
               if (RuntimeHelpers.IsReferenceOrContainsReferences<T>()) 
        
               { 
        
                   return new T[length]; 
        
               }

I considered that but I worried that garbage bits on reference fields would perhaps not be good news for the GC.

Seeing above, I'd say that making the pinned path forward to AllocateArray and return cleared memory makes sense here.

This was done by deferring reference types to GC.AllocateArray to avoid potential memory issues with the GC + uninitialized memory. The API only promises to avoid initialization if possible, anyway. Refactored tests to parametrically exercise these new relaxed constraints.

jthorborg · 2023-07-21T15:20:11Z

Okay, I've similarly relaxed AllocateUninitializedArray as well by delegating to AllocateArray if it's a reference type. I think it's a reasonable affordance for users, given it's not a promise to return uninitialized memory anyway from this API.

I refactored these tests and combined them to parametric ones to take this into account.

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

jkotas · 2023-07-21T16:27:09Z

Done, thanks I missed that. Are there tests to update for these as well?

The tests are shared for all runtime flavors. The test updates that you have done should be sufficient.

jthorborg · 2023-07-21T16:33:36Z

Done, thanks I missed that. Are there tests to update for these as well?

The tests are shared for all runtime flavors. The test updates that you have done should be sufficient.

Very nice!

cshung · 2023-07-21T16:54:39Z

@Maoni0 - this change relaxes the constraint that user POH allocation cannot contain references. This might impact our plan for #86802 meaning to fix #76929

Relying on internal implementation zeroing refs if necessary.

Mono already piggybacks on the AllocateArray path anyway.

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

Maoni0

other than my nit comment it LGTM! thanks for doing this work @jthorborg!

Co-authored-by: jthorborg <janus.thorborg@gmail.com>

jkotas · 2023-08-02T00:20:24Z

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs


            GC_ALLOC_FLAGS flags = GC_ALLOC_FLAGS.GC_ALLOC_ZEROING_OPTIONAL;
            if (pinned)
                flags |= GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP;

-            return Unsafe.As<T[]>(AllocateNewArray(RuntimeTypeHandle.ToIntPtr(typeof(T[]).TypeHandle), length, flags));
+            // Runtime overrides GC_ALLOC_ZEROING_OPTIONAL if the type contains references, so we don't need to worry about that.
+            return Unsafe.As<T[]>(AllocateNewArray(typeof(T[]).TypeHandle.Value, length, flags));


Why did you change RuntimeTypeHandle.ToIntPtr to RuntimeTypeHandle.Value?

RuntimeTypeHandle.ToIntPtr is JIT intrinsic so it will be optimized to just loading a constant in most cases. RuntimeTypeHandle.Value does not have that optimization. This change may cause a perf regression.

Oh! I had no idea it was an intrinsic, how is that discoverable?

I changed it to be identical to the similar usage just below: https://github.com/dotnet/runtime/pull/89293/files/d7dfad7341d828be78b09cbe1e49e696d09fac4c#diff-63ddcf9eb1f63c1ddef526934b79b086b1d30883f3293847bf966d047bc7666dR781

Perhaps the should instead revert this line and change the other to use the intrinsic?

Oh! I had no idea it was an intrinsic, how is that discoverable?

JIT intrinsics are marked with [Intrinsic] attributes.

Perhaps the should instead revert this line and change the other to use the intrinsic?

Agree.

Ok, I'll do the change once I get closure on the other discussion

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

src/libraries/System.Runtime/tests/System/GCTests.cs

src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs

…rray allocation that should be slightly faster.

After a longer discussion, settled on a slightly augmented suggestion that isn't as controversial as the prior one.

MichalPetryka · 2023-08-10T16:46:02Z

I'd maybe remove the if here

runtime/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

Lines 740 to 743 in 349970a

    
           if (RuntimeHelpers.IsReferenceOrContainsReferences<T>()) 
        
           { 
        
               return new T[length]; 
        
           }

jkotas · 2023-08-10T17:09:43Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs

@@ -837,7 +830,7 @@ static T[] AllocateNewUninitializedArray(int length, bool pinned)
                    throw new OverflowException();

                T[]? array = null;
-                RuntimeImports.RhAllocateNewArray(EETypePtr.EETypePtrOf<T[]>().RawValue, (uint)length, (uint)flags, Unsafe.AsPointer(ref array));
+                RuntimeImports.RhAllocateNewArray(MethodTable.Of<T[]>(), (uint)length, (uint)flags, Unsafe.AsPointer(ref array));


Could you please also change the type of RhAllocateNewArray argument to MethodTable* to fix the build break

src/libraries/System.Runtime/tests/System/GCTests.cs

AaronRobinsonMSFT · 2023-08-10T17:55:22Z

src/libraries/System.Runtime/tests/System/GCTests.cs

+            // This isn't a guarantee since CoreCLR reorders fields and ignores the implicit sequential consistency of
+            // structs, but it will never hurt the test.


Suggested change

// This isn't a guarantee since CoreCLR reorders fields and ignores the implicit sequential consistency of

// structs, but it will never hurt the test.

// The CLR is permitted to reorder fields and ignore the implicit sequential consistency of

// value types if they contain managed references, but it will never hurt the test.

jthorborg · 2023-08-10T18:02:45Z

I'd maybe remove the if here

runtime/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

Lines 740 to 743 in 349970a

if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())

{

return new T[length];

}

Wouldn't that leave dead code just below when !DEBUG? Or are you saying remove the whole block instead of just the conditional?

runtime/src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

Lines 745 to 755 in 349970a

    
                           // for debug builds we always want to call AllocateNewArray to detect AllocateNewArray bugs 
        
           #if !DEBUG 
        
                           // small arrays are allocated using `new[]` as that is generally faster. 
        
           #pragma warning disable 8500 // sizeof of managed types 
        
                           if (length < 2048 / sizeof(T)) 
        
           #pragma warning restore 8500 
        
                           { 
        
                               return new T[length]; 
        
                           } 
        
           #endif

jkotas · 2023-08-10T18:50:23Z

Wouldn't that leave dead code just below when !DEBUG? Or are you saying remove the whole block instead of just the conditional?

It looks like that this was fine-tuned based on microbenchmarks. If you delete anything here, you will most likely introduce regressions in micro-benchmarks that exercise this API.

I do not think that this is worth touching. You would have to come up with perf numbers that demonstrate that you are not regressing anything that matters.

* Fixing signature for `RhAllocateNewArray` in NativeAOT to directly use a `MethodTable*` * Adding explicit structlayout to silence warning for EmbeddedValueType in GCTests, and improved the comment.

jkotas · 2023-08-11T21:48:35Z

The wasm build failure is known infrastructure error according to the build analysis.

jkotas · 2023-08-11T21:50:42Z

Thank you!

jthorborg · 2023-08-11T23:52:33Z

Thank you, especially for the handholding while making green contributions. I'm sure it will be smoother the next time I'll have the pleasure.

However trivial this may seem, it will mean a great deal to us.

jthorborg added 2 commits July 21, 2023 12:53

Relaxing constraints of System.GC.AllocateArray<T>.

d579879

It's now possible to allocate pinned and default-initialized arrays that contain references. Fix #48703

Adding coverage of GC.AllocateArray with ref types.

5fc9157

Added a test that pins a reference type array and resolves references to indices with pointer arithmetic, checking that modifications are visible back in the original array. Test #48703

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 21, 2023

jkotas added the area-GC-coreclr label Jul 21, 2023

jkotas requested a review from Maoni0 July 21, 2023 11:14

Mono/NativeAOT relaxing of System.GC.AllocateArray<T>.

376fb89

jthorborg requested review from marek-safar and MichalStrehovsky as code owners July 21, 2023 11:47

jthorborg and others added 2 commits July 21, 2023 17:16

Merge branch 'dotnet:main' into poh-allocate-array-allow-reference-types

808d582

AaronRobinsonMSFT reviewed Jul 21, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs Outdated Show resolved Hide resolved

jthorborg added 2 commits July 21, 2023 19:34

Simplifiy conditionals in AllocateUninitializedArray.

c403266

Relying on internal implementation zeroing refs if necessary.

Also simplify path for NativeOAT.

30fd375

Mono already piggybacks on the AllocateArray path anyway.

MichalPetryka reviewed Jul 21, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs Show resolved Hide resolved

Maoni0 approved these changes Jul 24, 2023

View reviewed changes

Update src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

d7dfad7

Co-authored-by: jthorborg <janus.thorborg@gmail.com>

jkotas reviewed Aug 2, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs Show resolved Hide resolved

jkotas reviewed Aug 2, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

3a1bca6

jkotas reviewed Aug 7, 2023

View reviewed changes

src/libraries/System.Runtime/tests/System/GCTests.cs Outdated Show resolved Hide resolved

jkotas reviewed Aug 7, 2023

View reviewed changes

src/libraries/System.Runtime/tests/System/GCTests.cs Outdated Show resolved Hide resolved

jkotas reviewed Aug 7, 2023

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs Show resolved Hide resolved

jthorborg added 3 commits August 10, 2023 18:33

Don't use var if type name doesn't exist explicitly on right-hand side

4b805e5

PR feedback for using terse method tables and JIT intrinsics for GC a…

f3aff36

…rray allocation that should be slightly faster.

Changed EmbeddedValueType and added comment.

349970a

After a longer discussion, settled on a slightly augmented suggestion that isn't as controversial as the prior one.

jthorborg requested review from jkotas, AaronRobinsonMSFT and MichalPetryka August 10, 2023 16:38

jkotas reviewed Aug 10, 2023

View reviewed changes

src/libraries/System.Runtime/tests/System/GCTests.cs Show resolved Hide resolved

AaronRobinsonMSFT reviewed Aug 10, 2023

View reviewed changes

Suggestions from PR feedback.

bfe62ec

* Fixing signature for `RhAllocateNewArray` in NativeAOT to directly use a `MethodTable*` * Adding explicit structlayout to silence warning for EmbeddedValueType in GCTests, and improved the comment.

build-analysis bot mentioned this pull request Aug 11, 2023

Insufficient memory of docker containers on CI dotnet/dnceng#450

Open

3 tasks

jkotas merged commit b22aa17 into dotnet:main Aug 11, 2023
167 of 169 checks passed

jthorborg deleted the poh-allocate-array-allow-reference-types branch August 11, 2023 23:52

ghost locked as resolved and limited conversation to collaborators Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow reference types for pinned GC.AllocateArray() #89293

Allow reference types for pinned GC.AllocateArray() #89293

jthorborg commented Jul 21, 2023

jthorborg commented Jul 21, 2023

jkotas commented Jul 21, 2023

ghost commented Jul 21, 2023

jthorborg commented Jul 21, 2023

MichalPetryka commented Jul 21, 2023

jthorborg commented Jul 21, 2023

EgorBo commented Jul 21, 2023 •

edited

Loading

MichalPetryka commented Jul 21, 2023

jthorborg commented Jul 21, 2023 •

edited

Loading

jkotas commented Jul 21, 2023

jthorborg commented Jul 21, 2023

cshung commented Jul 21, 2023 •

edited

Loading

Maoni0 left a comment

jkotas Aug 2, 2023

jthorborg Aug 7, 2023

jkotas Aug 7, 2023

jthorborg Aug 7, 2023

MichalPetryka commented Aug 10, 2023

jkotas Aug 10, 2023

AaronRobinsonMSFT Aug 10, 2023

jthorborg commented Aug 10, 2023

jkotas commented Aug 10, 2023

jkotas commented Aug 11, 2023

jkotas commented Aug 11, 2023

jthorborg commented Aug 11, 2023

		// This isn't a guarantee since CoreCLR reorders fields and ignores the implicit sequential consistency of
		// structs, but it will never hurt the test.

Allow reference types for pinned GC.AllocateArray() #89293

Allow reference types for pinned GC.AllocateArray() #89293

Conversation

jthorborg commented Jul 21, 2023

jthorborg commented Jul 21, 2023

jkotas commented Jul 21, 2023

ghost commented Jul 21, 2023

jthorborg commented Jul 21, 2023

MichalPetryka commented Jul 21, 2023

jthorborg commented Jul 21, 2023

EgorBo commented Jul 21, 2023 • edited Loading

MichalPetryka commented Jul 21, 2023

jthorborg commented Jul 21, 2023 • edited Loading

jkotas commented Jul 21, 2023

jthorborg commented Jul 21, 2023

cshung commented Jul 21, 2023 • edited Loading

Maoni0 left a comment

Choose a reason for hiding this comment

jkotas Aug 2, 2023

Choose a reason for hiding this comment

jthorborg Aug 7, 2023

Choose a reason for hiding this comment

jkotas Aug 7, 2023

Choose a reason for hiding this comment

jthorborg Aug 7, 2023

Choose a reason for hiding this comment

MichalPetryka commented Aug 10, 2023

jkotas Aug 10, 2023

Choose a reason for hiding this comment

AaronRobinsonMSFT Aug 10, 2023

Choose a reason for hiding this comment

jthorborg commented Aug 10, 2023

jkotas commented Aug 10, 2023

jkotas commented Aug 11, 2023

jkotas commented Aug 11, 2023

jthorborg commented Aug 11, 2023

EgorBo commented Jul 21, 2023 •

edited

Loading

jthorborg commented Jul 21, 2023 •

edited

Loading

cshung commented Jul 21, 2023 •

edited

Loading