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

Have CoreCLR follow sequential layout with GC refs #18

Closed
wants to merge 2 commits into from

Conversation

scott-ferguson-unity
Copy link

By default CoreCLR will ignore the LayoutKind.Sequential on
types containing GC references since those types are not
blittable and no code outside of the runtime can access those types
and will use the auto layout algorithm to change the field layout.

But Unity uses a lot of custom icalls that rely on the managed layout
matching the native layout.

This PR adds an IsSequentialWithRefs flag on MethodTable to indicate
the types with sequential layout, but we GC refs:

  1. The type has the LayoutKind.Sequential flag
  2. The type is not blittable and not managed sequential (not blittable but has no GC refs). These are already handled specially by the runtime.
  3. Has a parent that is System.Object, System.ValueType, is sequential with refs, is blittable, or is managed sequential

This requires only very small changes in the runtime because the explicit layout paths do the work we need already.

  1. classlayoutinfo.cpp already calculates field size and sequential offsets so we only need to mark the types as sequential with refs there.
  2. In methodtablebuilder.cpp we need to set the field offset to the value calculated in 1.
  3. In methodtablebuilder.cpp we run HandleExplicitLayout() which will calculate the type's size (it also, needlessly in our case, validates the field offsets)
  4. In methodtablebuilder.cpp we run HandleGCForExplicitLayout() which calculates the GCDesc for the type.

The only other needed change was to change the definition of MetadataEnumResult in managedmdimport.hpp.
This type is used in an icall that assumes the auto-layout algorithm - interestingly it has nothing to do with GC
refs, but the fact that the auto-layout algorithm places larger than pointer sized structs on pointer sized boundaries.

This PR also updates CrossGen2 so follows the same layout algorithm. The runtime has a lot of checks that
validate that the R2R image match the current runtime layout, size, and GC desc.

By default CoreCLR will ignore the LayoutKind.Sequential on
types containing GC references since those types are not
blittable and no code outside of the runtime can access those types
and will use the auto layout algorithm to change the field layout.

But Unity uses a lot of custom icalls that rely on the managed layout
matching the native layout.

This PR adds an IsSequentialWithRefs flag on MethodTable to indicate
the types with sequential layout, but we GC refs.  These types:

1. Have the LayoutKind.Sequential flag
2. The type is not blittable and not managed sequential (not blittable but has no GC refs).  These are already handled specially by the runtime.
3. Has a parent that is System.Object, System.ValueType, is sequential with refs, is blittable, or is managed sequential

This requires only very small changes in the runtime because the explicit layout paths do the work we need already.

1. classlayoutinfo.cpp already calculates field size and sequential offsets so we only need to mark the types as sequential with refs there.
2. In methodtablebuilder.cpp we need to set the field offset to the value calculated in 1.
3. In methodtablebuilder.cpp we run HandleExplicitLayout() which will calculate the type's size (it also, needlessly in our case, validates the field offsets)
4. In methodtablebuilder.cpp we run HandleGCForExplicitLayout() which calculates the GCDesc for the type.

The only other needed change was to change the definition of MetadataEnumResult in managedmdimport.hpp.
This type is used in an icall that assumes the auto-layout algorithm - interestingly it has nothing to do with GC
refs, but the fact that the auto-layout algorithm places larger than pointer sized structs on pointer sized boundaries.

This PR also updates CrossGen2 so follows the same layout algorithm.  The runtime has a lot of checks that
validate that the R2R image match the current runtime layout, size, and GC desc.
@joshpeterson
Copy link

We will track the work to undo this change when we can correct things on the Unity side here: #17

{
return ComputeSequentialFieldLayout(type, numInstanceFields, isSequentialWithRefs);
}
#else
if (type.IsEnum || MarshalUtils.IsBlittableType(type) || IsManagedSequentialType(type))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this logic got a major overhaul in dotnet#61759 . I think you will want to do matching cleanup of FEATURE_SEQUENTIAL_LAYOUT_WITH_REFS mode once you pick up that change. I hope that it will make things simpler overall.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting us know about this pull request, it looks very useful!

yuc434 pushed a commit that referenced this pull request Aug 8, 2022
* Initial implementation for contract customization

fix build errors

Move converter rooting to DefaultJsonTypeInfoResolver so that it can be used standalone

Fix ConfigurationList.IsReadOnly

Minor refactorings (#1)

* Makes the following changes:

* Move singleton initialization for DefaultTypeInfoResolver behind a static property.
* Consolidate JsonSerializerContext & IJsonTypeInfoResolver values to a single field.
* Move reflection fallback logic away from JsonSerializerContext and into JsonSerializerOptions

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

* remove testing of removed field

Simplify the JsonTypeInfo.CreateObject implemenetation (#2)

* Simplify the JsonTypeInfo.CreateObject implemenetation

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs

Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com>

Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com>

Tests and fixes for JsonTypeInfoKind.None

TypeInfo type mismatch tests

Allow setting NumberHandling on JsonTypeInfoKind.None

test resolver returning wrong type of options

JsonTypeInfo/JsonPropertyInfo mutability tests

rename test file

Move default converter rooting responsibility behind DefaultJsonTypeInfoResolver (#3)

* Move default converter rooting responsibility behind DefaultJsonTypeInfoResolver

* address feedback

Add simple test for using JsonTypeInfo<T> with APIs directly taking it

fix and tests for untyped/typed CreateObject

uncomment test cases, remove todo

More tests and tiny fixes

Add a JsonTypeInfoResolver.Combine test for JsonSerializerContext (#4)

* Fix JsonTypeInfoResolver.Combine for JsonSerializerContext

* Break up failing test

Fix simple scenarios for combining contexts (#6)

* Fix simple scenarios for combining contexts

* feedback

JsonSerializerContext combine test with different camel casing

Remove unneeded virtual calls & branching when accessing Get & Set delegates (#7)

JsonPropertyInfo tests everything minus ShouldSerialize & NumberHandling

Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

throw InvalidOperationException rather than ArgumentNullException for source gen when PropertyInfo.Name is assigned through JsonPropertyInfoValues

tests for duplicated property names and JsonPropertyInfo.NumberHandling

Add tests for NumberHandling and failing tests for ShouldSerialize

disable the failing test and add extra checks

disable remainder of the failing ShouldSerialize tests, fix working one

Fix ShouldSerialize and IgnoreCondition interop

Add failing tests for CreateObject + parametrized constructors

Fix CreateObject support for JsonConstructor types (#10)

* Fix CreateObject support for JsonConstructor types

* address feedback

Make contexts more combinator friendly (#9)

* Make contexts more combinator friendly

* remove converter cache

* redesign test to account for JsonConstructorAttribute

* Combine unit tests

* address feedback

* Add acceptance tests for DataContract attributes & Specified pattern (#11)

* Add private field serialization acceptance test (#13)

* tests, PR feedback (#14)

* PR feedback and extra tests

* Shorten class name, remove incorrect check (not true for polimorphic cases)

* Make parameter matching for custom properties map property Name with parameter (#16)

* Test static initialization with JsonTypeInfo (#17)

* Fix test failures and proper fix this time (#18)

* Fix test failures and proper fix this time

* reinstate ActiveIssueAttribute

* PR feedback and adjust couple of tests which don't set TypeInfoResolver

* fix IAsyncEnumerable tests

* Lock JsonSerializerOptions in JsonTypeInfo.EnsureConfigured()

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
@UnityAlex UnityAlex deleted the branch unity-main January 4, 2024 14:25
@UnityAlex UnityAlex closed this Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants