From a074418e34bc9d943f3b91ffacc15c249527cb1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 22 Feb 2018 11:00:38 +0100 Subject: [PATCH 1/7] WIP --- ...ypeGetFieldHelperMethodOverride.Sorting.cs | 20 +++ .../ValueTypeGetFieldHelperMethodOverride.cs | 140 ++++++++++++++++++ .../IL/TypeSystemContext.ValueTypeMethods.cs | 48 ++++++ .../src/Compiler/CompilerTypeSystemContext.cs | 4 + .../src/ILCompiler.TypeSystem.csproj | 9 ++ .../src/System/Runtime/RuntimeImports.cs | 4 + .../src/System/ValueType.cs | 79 ++++++++++ 7 files changed, 304 insertions(+) create mode 100644 src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.Sorting.cs create mode 100644 src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs create mode 100644 src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs diff --git a/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.Sorting.cs b/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.Sorting.cs new file mode 100644 index 00000000000..4122b8c3cd7 --- /dev/null +++ b/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.Sorting.cs @@ -0,0 +1,20 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using Internal.TypeSystem; + +namespace Internal.IL.Stubs +{ + partial class ValueTypeGetFieldHelperMethodOverride + { + protected internal override int ClassCode => 2036839816; + + protected internal override int CompareToImpl(MethodDesc other, TypeSystemComparer comparer) + { + var otherMethod = (ValueTypeGetFieldHelperMethodOverride)other; + + return comparer.Compare(_owningType, otherMethod._owningType); + } + } +} diff --git a/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs b/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs new file mode 100644 index 00000000000..42562652630 --- /dev/null +++ b/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs @@ -0,0 +1,140 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; + +using Internal.TypeSystem; + +namespace Internal.IL.Stubs +{ + /// + /// Synthetic method override of "IntPtr Delegate.GetThunk(Int32)". This method is injected + /// into all delegate types and provides means for System.Delegate to access the various thunks + /// generated by the compiler. + /// + public sealed partial class ValueTypeGetFieldHelperMethodOverride : ILStubMethod + { + private DefType _owningType; + private MethodSignature _signature; + + internal ValueTypeGetFieldHelperMethodOverride(DefType owningType) + { + _owningType = owningType; + } + + public override TypeSystemContext Context + { + get + { + return _owningType.Context; + } + } + + public override TypeDesc OwningType + { + get + { + return _owningType; + } + } + + public override MethodSignature Signature + { + get + { + if (_signature == null) + { + TypeSystemContext context = _owningType.Context; + TypeDesc int32Type = context.GetWellKnownType(WellKnownType.Int32); + TypeDesc eeTypePtrType = context.SystemModule.GetKnownType("System", "EETypePtr"); + + _signature = new MethodSignature(0, 0, int32Type, new[] { + int32Type, + eeTypePtrType.MakeByRefType() + }); + } + + return _signature; + } + } + + public override MethodIL EmitIL() + { + TypeDesc owningType = _owningType.InstantiateAsOpen(); + + ILEmitter emitter = new ILEmitter(); + + TypeDesc eeTypePtrType = Context.SystemModule.GetKnownType("System", "EETypePtr"); + MethodDesc eeTypePtrOfMethod = eeTypePtrType.GetKnownMethod("EETypePtrOf", null); + ILToken eeTypePtrToken = emitter.NewToken(eeTypePtrType); + + var switchStream = emitter.NewCodeStream(); + var getFieldStream = emitter.NewCodeStream(); + + ArrayBuilder fieldGetters = new ArrayBuilder(); + foreach (FieldDesc field in owningType.GetFields()) + { + if (field.IsStatic) + continue; + + ILCodeLabel label = emitter.NewCodeLabel(); + fieldGetters.Add(label); + + getFieldStream.EmitLabel(label); + getFieldStream.EmitLdArg(2); + + TypeDesc boxableFieldType = field.FieldType; + if (boxableFieldType.IsPointer || boxableFieldType.IsFunctionPointer) + boxableFieldType = Context.GetWellKnownType(WellKnownType.IntPtr); + + MethodDesc ptrOfField = eeTypePtrOfMethod.MakeInstantiatedMethod(boxableFieldType); + getFieldStream.Emit(ILOpcode.call, emitter.NewToken(ptrOfField)); + + getFieldStream.Emit(ILOpcode.stobj, eeTypePtrToken); + + getFieldStream.EmitLdArg(0); + getFieldStream.Emit(ILOpcode.ldflda, emitter.NewToken(field)); + + getFieldStream.EmitLdArg(0); + + getFieldStream.Emit(ILOpcode.sub); + + getFieldStream.Emit(ILOpcode.ret); + } + + switchStream.EmitLdArg(1); + switchStream.EmitSwitch(fieldGetters.ToArray()); + + switchStream.EmitLdc(fieldGetters.Count); + + switchStream.Emit(ILOpcode.ret); + + return emitter.Link(this); + } + + public override Instantiation Instantiation + { + get + { + return Instantiation.Empty; + } + } + + public override bool IsVirtual + { + get + { + return true; + } + } + + public override string Name + { + get + { + return "__GetFieldHelper"; + } + } + } +} diff --git a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs new file mode 100644 index 00000000000..94a8f622794 --- /dev/null +++ b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs @@ -0,0 +1,48 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; + +using Internal.IL.Stubs; + +using Debug = System.Diagnostics.Debug; + +namespace Internal.TypeSystem +{ + public abstract partial class TypeSystemContext + { + private class ValueTypeMethodHashtable : LockFreeReaderHashtable + { + protected override int GetKeyHashCode(DefType key) => key.GetHashCode(); + protected override int GetValueHashCode(MethodDesc value) => value.OwningType.GetHashCode(); + protected override bool CompareKeyToValue(DefType key, MethodDesc value) => key == value.OwningType; + protected override bool CompareValueToValue(MethodDesc v1, MethodDesc v2) => v1.OwningType == v2.OwningType; + + protected override MethodDesc CreateValueFromKey(DefType key) + { + return new ValueTypeGetFieldHelperMethodOverride(key); + } + } + + private ValueTypeMethodHashtable _valueTypeMethodHashtable = new ValueTypeMethodHashtable(); + + protected virtual IEnumerable GetAllMethodsForValueType(TypeDesc valueType) + { + TypeDesc valueTypeDefinition = valueType.GetTypeDefinition(); + MethodDesc getFieldHelperMethod = _valueTypeMethodHashtable.GetOrCreateValue((DefType)valueTypeDefinition); + + if (valueType != valueTypeDefinition) + { + yield return GetMethodForInstantiatedType(getFieldHelperMethod, (InstantiatedType)valueType); + } + else + { + yield return getFieldHelperMethod; + } + + foreach (MethodDesc method in valueType.GetMethods()) + yield return method; + } + } +} diff --git a/src/ILCompiler.Compiler/src/Compiler/CompilerTypeSystemContext.cs b/src/ILCompiler.Compiler/src/Compiler/CompilerTypeSystemContext.cs index b224f558fd9..dfb160c119e 100644 --- a/src/ILCompiler.Compiler/src/Compiler/CompilerTypeSystemContext.cs +++ b/src/ILCompiler.Compiler/src/Compiler/CompilerTypeSystemContext.cs @@ -345,6 +345,10 @@ protected override IEnumerable GetAllMethods(TypeDesc type) { return GetAllMethodsForEnum(type); } + else if (type.IsValueType) + { + return GetAllMethodsForValueType(type); + } return type.GetMethods(); } diff --git a/src/ILCompiler.TypeSystem/src/ILCompiler.TypeSystem.csproj b/src/ILCompiler.TypeSystem/src/ILCompiler.TypeSystem.csproj index d9de5b3c90c..c9b2ef2bfde 100644 --- a/src/ILCompiler.TypeSystem/src/ILCompiler.TypeSystem.csproj +++ b/src/ILCompiler.TypeSystem/src/ILCompiler.TypeSystem.csproj @@ -379,6 +379,12 @@ IL\Stubs\StructMarshallingThunk.Sorting.cs + + IL\Stubs\ValueTypeGetFieldHelperMethodOverride.cs + + + IL\Stubs\ValueTypeGetFieldHelperMethodOverride.Sorting.cs + IL\TypeSystemContext.DelegateInfo.cs @@ -418,6 +424,9 @@ IL\TypeSystemContext.EnumMethods.cs + + IL\TypeSystemContext.ValueTypeMethods.cs + TypeSystem\Interop\IL\InlineArrayType.Sorting.cs diff --git a/src/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs b/src/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs index e227b7fa635..f8ebc13ee81 100644 --- a/src/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs +++ b/src/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs @@ -332,6 +332,10 @@ internal static unsafe Object RhHandleGet(IntPtr handle) [RuntimeImport(RuntimeLibrary, "RhBoxAny")] internal static extern unsafe object RhBoxAny(void* pData, EETypePtr pEEType); + [MethodImpl(MethodImplOptions.InternalCall)] + [RuntimeImport(RuntimeLibrary, "RhBoxAny")] + internal static extern unsafe object RhBoxAny(ref byte pData, EETypePtr pEEType); + [MethodImpl(MethodImplOptions.InternalCall)] [RuntimeImport(RuntimeLibrary, "RhNewObject")] internal static extern object RhNewObject(EETypePtr pEEType); diff --git a/src/System.Private.CoreLib/src/System/ValueType.cs b/src/System.Private.CoreLib/src/System/ValueType.cs index d75dd2692ed..000ab98f97a 100644 --- a/src/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/System.Private.CoreLib/src/System/ValueType.cs @@ -11,8 +11,13 @@ ** ===========================================================*/ +using System.Runtime; + +using Internal.Runtime.CompilerServices; using Internal.Runtime.Augments; +using Debug = System.Diagnostics.Debug; + namespace System { // CONTRACT with Runtime @@ -26,6 +31,7 @@ public override String ToString() return this.GetType().ToString(); } +#if PROJECTN public override bool Equals(object obj) { return RuntimeAugments.Callbacks.ValueTypeEqualsUsingReflection(this, obj); @@ -36,4 +42,77 @@ public override int GetHashCode() return RuntimeAugments.Callbacks.ValueTypeGetHashCodeUsingReflection(this); } } +#else + private const int UseFastHelper = -1; + private const int GetNumFields = -1; + + // An override of this method will be injected by the compiler into all valuetypes that cannot be compared + // using a simple memory comparison. + // This API is a bit awkward because we want to avoid burning more than one vtable slot on this. + // When index == GetNumFields, this method is expected to return the number of fields of this + // valuetype. Otherwise, it returns the offset and type handle of the index-th field on this type. + internal virtual int __GetFieldHelper(int index, out EETypePtr eeType) + { + // Value types that don't override this method will use the fast path that looks at bytes, not fields. + Debug.Assert(index == GetNumFields); + eeType = default; + return UseFastHelper; + } + + public override bool Equals(object obj) + { + if (obj == null || obj.EETypePtr != this.EETypePtr) + return false; + + int numFields = __GetFieldHelper(GetNumFields, out _); + + ref byte thisRawData = ref this.GetRawData(); + ref byte thatRawData = ref obj.GetRawData(); + + if (numFields == UseFastHelper) + { + // Sanity check - if there are GC references, we should not be comparing bytes + Debug.Assert(RuntimeImports.RhGetGCDescSize(this.EETypePtr) == 0); + + // Compare the memory + int valueTypeSize = (int)this.EETypePtr.ValueTypeSize; + for (int i = 0; i < valueTypeSize; i++) + { + if (Unsafe.Add(ref thisRawData, i) != Unsafe.Add(ref thatRawData, i)) + return false; + } + } + else + { + // Foreach field, box and call the Equals method. + for (int i = 0; i < numFields; i++) + { + int fieldOffset = __GetFieldHelper(i, out EETypePtr fieldType); + + // Fetch the value of the field on both types + object thisField = RuntimeImports.RhBoxAny(ref Unsafe.Add(ref thisRawData, fieldOffset), fieldType); + object thatField = RuntimeImports.RhBoxAny(ref Unsafe.Add(ref thatRawData, fieldOffset), fieldType); + + // Compare the fields + if (thisField == null) + { + if (thatField != null) + return false; + } + else if (!thisField.Equals(thatField)) + { + return false; + } + } + } + + return true; + } + + public override int GetHashCode() + { + throw new NotImplementedException(); + } +#endif + } } From 432a1d36301332b92277163cf42f259bdbd46257 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 23 Feb 2018 11:03:26 +0100 Subject: [PATCH 2/7] WIP --- .../ValueTypeGetFieldHelperMethodOverride.cs | 7 +- .../IL/TypeSystemContext.ValueTypeMethods.cs | 104 ++++++++++++++++-- .../src/System/ValueType.cs | 90 ++++++++++++++- 3 files changed, 190 insertions(+), 11 deletions(-) diff --git a/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs b/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs index 42562652630..69cf813d8e8 100644 --- a/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs +++ b/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs @@ -103,8 +103,11 @@ public override MethodIL EmitIL() getFieldStream.Emit(ILOpcode.ret); } - switchStream.EmitLdArg(1); - switchStream.EmitSwitch(fieldGetters.ToArray()); + if (fieldGetters.Count > 0) + { + switchStream.EmitLdArg(1); + switchStream.EmitSwitch(fieldGetters.ToArray()); + } switchStream.EmitLdc(fieldGetters.Count); diff --git a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs index 94a8f622794..9ee9bfbe350 100644 --- a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs +++ b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs @@ -12,6 +12,9 @@ namespace Internal.TypeSystem { public abstract partial class TypeSystemContext { + private MethodDesc _objectGetHashCodeMethod; + private MethodDesc _objectEqualsMethod; + private class ValueTypeMethodHashtable : LockFreeReaderHashtable { protected override int GetKeyHashCode(DefType key) => key.GetHashCode(); @@ -30,19 +33,106 @@ protected override MethodDesc CreateValueFromKey(DefType key) protected virtual IEnumerable GetAllMethodsForValueType(TypeDesc valueType) { TypeDesc valueTypeDefinition = valueType.GetTypeDefinition(); - MethodDesc getFieldHelperMethod = _valueTypeMethodHashtable.GetOrCreateValue((DefType)valueTypeDefinition); - if (valueType != valueTypeDefinition) - { - yield return GetMethodForInstantiatedType(getFieldHelperMethod, (InstantiatedType)valueType); - } - else + if (RequiresGetFieldHelperMethod((MetadataType)valueTypeDefinition)) { - yield return getFieldHelperMethod; + MethodDesc getFieldHelperMethod = _valueTypeMethodHashtable.GetOrCreateValue((DefType)valueTypeDefinition); + + // Check that System.ValueType has the method we're overriding. + Debug.Assert(valueTypeDefinition.BaseType.GetMethod(getFieldHelperMethod.Name, null) != null); + + if (valueType != valueTypeDefinition) + { + yield return GetMethodForInstantiatedType(getFieldHelperMethod, (InstantiatedType)valueType); + } + else + { + yield return getFieldHelperMethod; + } } foreach (MethodDesc method in valueType.GetMethods()) yield return method; } + + private bool RequiresGetFieldHelperMethod(MetadataType valueType) + { + if (_objectGetHashCodeMethod == null) + _objectGetHashCodeMethod = GetWellKnownType(WellKnownType.Object).GetMethod("GetHashCode", null); + + if (_objectEqualsMethod == null) + _objectEqualsMethod = GetWellKnownType(WellKnownType.Object).GetMethod("Equals", null); + + // If the classlib doesn't have Object.Equals/Object.GetHashCode, we don't need this. + if (_objectEqualsMethod == null && _objectGetHashCodeMethod == null) + return false; + + // Byref-like valuetypes cannot be boxed. + if (valueType.IsByRefLike) + return false; + + // Enums get their overrides from System.Enum. + if (valueType.IsEnum) + return false; + + return !CanCompareValueTypeBits(valueType); + } + + private bool CanCompareValueTypeBits(MetadataType type) + { + Debug.Assert(type.IsValueType); + + if (type.ContainsGCPointers) + return false; + + // TODO: what we're shooting for is overlapping fields + // or gaps between fields + if (type.IsExplicitLayout || type.GetClassLayout().Size != 0) + return false; + + bool result = true; + foreach (var field in type.GetFields()) + { + if (field.IsStatic) + continue; + + TypeDesc fieldType = field.FieldType; + if (fieldType.IsPrimitive || fieldType.IsEnum || fieldType.IsPointer || fieldType.IsFunctionPointer) + { + TypeFlags category = fieldType.UnderlyingType.Category; + if (category == TypeFlags.Single || category == TypeFlags.Double) + { + // Double/Single have weird behaviors around negative/positive zero + result = false; + break; + } + } + else if (fieldType.IsSignatureVariable) + { + return false; + } + else + { + // Would be a suprise if this wasn't a valuetype. We checked ContainsGCPointers above. + Debug.Assert(fieldType.IsValueType); + + // If the field overrides Equals/GetHashCode, we can't use the fast helper because we need to call the method. + if (fieldType.FindVirtualFunctionTargetMethodOnObjectType(_objectEqualsMethod) != null || + fieldType.FindVirtualFunctionTargetMethodOnObjectType(_objectGetHashCodeMethod) != null) + { + result = false; + break; + } + + if (!CanCompareValueTypeBits((MetadataType)fieldType)) + { + result = false; + break; + } + } + } + + return result; + } } } diff --git a/src/System.Private.CoreLib/src/System/ValueType.cs b/src/System.Private.CoreLib/src/System/ValueType.cs index 000ab98f97a..6d7b04c42f8 100644 --- a/src/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/System.Private.CoreLib/src/System/ValueType.cs @@ -72,7 +72,7 @@ public override bool Equals(object obj) if (numFields == UseFastHelper) { // Sanity check - if there are GC references, we should not be comparing bytes - Debug.Assert(RuntimeImports.RhGetGCDescSize(this.EETypePtr) == 0); + Debug.Assert(!this.EETypePtr.HasPointers); // Compare the memory int valueTypeSize = (int)this.EETypePtr.ValueTypeSize; @@ -111,7 +111,93 @@ public override bool Equals(object obj) public override int GetHashCode() { - throw new NotImplementedException(); + int hashCode = this.EETypePtr.GetHashCode(); + + int numFields = __GetFieldHelper(GetNumFields, out _); + + if (numFields == UseFastHelper) + { + hashCode ^= FastGetValueTypeHashCodeHelper(this.EETypePtr, ref this.GetRawData()); + } + else + { + hashCode ^= RegularGetValueTypeHashCode(this.EETypePtr, ref this.GetRawData(), numFields); + } + + return hashCode; + } + + private static int FastGetValueTypeHashCodeHelper(EETypePtr type, ref byte data) + { + // Sanity check - if there are GC references, we should not be hashing bytes + Debug.Assert(!type.HasPointers); + + int size = (int)type.ValueTypeSize; + int hashCode = 0; + + for (int i = 0; i < size / 4; i++) + { + hashCode ^= Unsafe.As(ref Unsafe.Add(ref data, i * 4)); + } + + return hashCode; + } + + private int RegularGetValueTypeHashCode(EETypePtr type, ref byte data, int numFields) + { + int hashCode = 0; + + // We only take the hashcode for the first non-null field. That's what the CLR does. + for (int i = 0; i < numFields; i++) + { + int fieldOffset = __GetFieldHelper(i, out EETypePtr fieldType); + ref byte fieldData = ref Unsafe.Add(ref data, fieldOffset); + + if (fieldType.IsPointer) + { + hashCode = Unsafe.Read(ref fieldData).GetHashCode(); + } + else if (fieldType.CorElementType == RuntimeImports.RhCorElementType.ELEMENT_TYPE_R4) + { + hashCode = Unsafe.Read(ref fieldData).GetHashCode(); + } + else if (fieldType.CorElementType == RuntimeImports.RhCorElementType.ELEMENT_TYPE_R8) + { + hashCode = Unsafe.Read(ref fieldData).GetHashCode(); + } + else if (fieldType.IsPrimitive) + { + hashCode = FastGetValueTypeHashCodeHelper(fieldType, ref fieldData); + } + else if (fieldType.IsValueType) + { + // We have no option but to box and call regular GetHashCode since this value type could have + // GC pointers (we could find out if we want though), or fields of type Double/Single (we can't + // really find out). Double/Single have weird requirements around -0.0 and +0.0. + // If this boxing becomes a problem, we could build a piece of infrastructure that determines the slot + // of __GetFieldHelper, decodes the unboxing stub pointed to by the slot to the real target + // (we already have that part), and calls the entrypoint that expects a byref `this`, and use the + // data to decide between calling fast or regular hashcode helper. + object fieldValue = RuntimeImports.RhBox(fieldType, ref fieldData); + hashCode = fieldValue.GetHashCode(); + } + else + { + object fieldValue = Unsafe.Read(ref fieldData); + if (fieldValue != null) + { + hashCode = fieldValue.GetHashCode(); + } + else + { + // null object reference, try next + continue; + } + } + break; + } + + return hashCode; } #endif } From 044df31b7e9e4faf4e2f245b2ee144c031f33f4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 23 Feb 2018 11:54:30 +0100 Subject: [PATCH 3/7] WIP --- .../IL/TypeSystemContext.ValueTypeMethods.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs index 9ee9bfbe350..c35502dba36 100644 --- a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs +++ b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs @@ -75,6 +75,16 @@ private bool RequiresGetFieldHelperMethod(MetadataType valueType) if (valueType.IsEnum) return false; + // Optimization: if Equals/GetHashCode are overriden, we don't need the helper + // TODO: not stricly correct because user code can do + // public override int GetHashCode() => base.GetHashCode + // and cause us to not be able to provide the implementation anymore + // We should probably scope this down to e.g. only framework code. + bool overridesEquals = valueType.GetMethod("Equals", _objectEqualsMethod.Signature) != null; + bool overridesGetHashCode = valueType.GetMethod("GetHashCode", _objectGetHashCodeMethod.Signature) != null; + if (overridesEquals && overridesGetHashCode) + return false; + return !CanCompareValueTypeBits(valueType); } From 2812680cd4ff0b6bd25fdb20bcffdbe5c1df4bad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 23 Feb 2018 14:01:56 +0100 Subject: [PATCH 4/7] Not worth it --- .../IL/TypeSystemContext.ValueTypeMethods.cs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs index c35502dba36..9ee9bfbe350 100644 --- a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs +++ b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs @@ -75,16 +75,6 @@ private bool RequiresGetFieldHelperMethod(MetadataType valueType) if (valueType.IsEnum) return false; - // Optimization: if Equals/GetHashCode are overriden, we don't need the helper - // TODO: not stricly correct because user code can do - // public override int GetHashCode() => base.GetHashCode - // and cause us to not be able to provide the implementation anymore - // We should probably scope this down to e.g. only framework code. - bool overridesEquals = valueType.GetMethod("Equals", _objectEqualsMethod.Signature) != null; - bool overridesGetHashCode = valueType.GetMethod("GetHashCode", _objectGetHashCodeMethod.Signature) != null; - if (overridesEquals && overridesGetHashCode) - return false; - return !CanCompareValueTypeBits(valueType); } From 0790b8f5e14d32d7082c139249c53b9eb43fff3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 23 Feb 2018 14:14:42 +0100 Subject: [PATCH 5/7] Fixes --- .../IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs | 8 +++++--- src/System.Private.CoreLib/src/System/ValueType.cs | 8 +++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs b/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs index 69cf813d8e8..33d71794a60 100644 --- a/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs +++ b/src/Common/src/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs @@ -9,9 +9,9 @@ namespace Internal.IL.Stubs { /// - /// Synthetic method override of "IntPtr Delegate.GetThunk(Int32)". This method is injected - /// into all delegate types and provides means for System.Delegate to access the various thunks - /// generated by the compiler. + /// Synthetic method override of "int ValueType.__GetFieldHelper(Int32, out EETypePtr)". This method is injected + /// into all value types that cannot have their Equals(object) and GetHashCode() methods operate on individual + /// bytes. The purpose of the override is to provide access to the value types' fields and their types. /// public sealed partial class ValueTypeGetFieldHelperMethodOverride : ILStubMethod { @@ -84,6 +84,8 @@ public override MethodIL EmitIL() getFieldStream.EmitLabel(label); getFieldStream.EmitLdArg(2); + // We need something we can instantiate EETypePtrOf over. Also, the classlib + // code doesn't handle pointers. TypeDesc boxableFieldType = field.FieldType; if (boxableFieldType.IsPointer || boxableFieldType.IsFunctionPointer) boxableFieldType = Context.GetWellKnownType(WellKnownType.IntPtr); diff --git a/src/System.Private.CoreLib/src/System/ValueType.cs b/src/System.Private.CoreLib/src/System/ValueType.cs index 6d7b04c42f8..a3f9a2d051b 100644 --- a/src/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/System.Private.CoreLib/src/System/ValueType.cs @@ -153,11 +153,9 @@ private int RegularGetValueTypeHashCode(EETypePtr type, ref byte data, int numFi int fieldOffset = __GetFieldHelper(i, out EETypePtr fieldType); ref byte fieldData = ref Unsafe.Add(ref data, fieldOffset); - if (fieldType.IsPointer) - { - hashCode = Unsafe.Read(ref fieldData).GetHashCode(); - } - else if (fieldType.CorElementType == RuntimeImports.RhCorElementType.ELEMENT_TYPE_R4) + Debug.Assert(!fieldType.IsPointer); + + if (fieldType.CorElementType == RuntimeImports.RhCorElementType.ELEMENT_TYPE_R4) { hashCode = Unsafe.Read(ref fieldData).GetHashCode(); } From 0f0e0f9d42aaa348f1dab5de11be4f37493cd393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 26 Feb 2018 10:21:41 +0100 Subject: [PATCH 6/7] Update not to call GetHashCode --- .../src/System/ValueType.cs | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/ValueType.cs b/src/System.Private.CoreLib/src/System/ValueType.cs index a3f9a2d051b..b58306d58a2 100644 --- a/src/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/System.Private.CoreLib/src/System/ValueType.cs @@ -113,18 +113,19 @@ public override int GetHashCode() { int hashCode = this.EETypePtr.GetHashCode(); + hashCode ^= GetHashCodeImpl(); + + return hashCode; + } + + private int GetHashCodeImpl() + { int numFields = __GetFieldHelper(GetNumFields, out _); if (numFields == UseFastHelper) - { - hashCode ^= FastGetValueTypeHashCodeHelper(this.EETypePtr, ref this.GetRawData()); - } - else - { - hashCode ^= RegularGetValueTypeHashCode(this.EETypePtr, ref this.GetRawData(), numFields); - } + return FastGetValueTypeHashCodeHelper(this.EETypePtr, ref this.GetRawData()); - return hashCode; + return RegularGetValueTypeHashCode(this.EETypePtr, ref this.GetRawData(), numFields); } private static int FastGetValueTypeHashCodeHelper(EETypePtr type, ref byte data) @@ -169,15 +170,15 @@ private int RegularGetValueTypeHashCode(EETypePtr type, ref byte data, int numFi } else if (fieldType.IsValueType) { - // We have no option but to box and call regular GetHashCode since this value type could have + // We have no option but to box since this value type could have // GC pointers (we could find out if we want though), or fields of type Double/Single (we can't // really find out). Double/Single have weird requirements around -0.0 and +0.0. // If this boxing becomes a problem, we could build a piece of infrastructure that determines the slot // of __GetFieldHelper, decodes the unboxing stub pointed to by the slot to the real target // (we already have that part), and calls the entrypoint that expects a byref `this`, and use the // data to decide between calling fast or regular hashcode helper. - object fieldValue = RuntimeImports.RhBox(fieldType, ref fieldData); - hashCode = fieldValue.GetHashCode(); + var fieldValue = (ValueType)RuntimeImports.RhBox(fieldType, ref fieldData); + hashCode = fieldValue.GetHashCodeImpl(); } else { From 554d73ac0a72843a18a44ee3795c23c3149b0b4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 26 Feb 2018 11:17:49 +0100 Subject: [PATCH 7/7] Compute gaps and overlapping fields --- .../IL/TypeSystemContext.ValueTypeMethods.cs | 83 +++++++++++++++---- 1 file changed, 67 insertions(+), 16 deletions(-) diff --git a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs index 9ee9bfbe350..7106b1980c3 100644 --- a/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs +++ b/src/Common/src/TypeSystem/IL/TypeSystemContext.ValueTypeMethods.cs @@ -12,7 +12,6 @@ namespace Internal.TypeSystem { public abstract partial class TypeSystemContext { - private MethodDesc _objectGetHashCodeMethod; private MethodDesc _objectEqualsMethod; private class ValueTypeMethodHashtable : LockFreeReaderHashtable @@ -57,14 +56,11 @@ protected virtual IEnumerable GetAllMethodsForValueType(TypeDesc val private bool RequiresGetFieldHelperMethod(MetadataType valueType) { - if (_objectGetHashCodeMethod == null) - _objectGetHashCodeMethod = GetWellKnownType(WellKnownType.Object).GetMethod("GetHashCode", null); - if (_objectEqualsMethod == null) _objectEqualsMethod = GetWellKnownType(WellKnownType.Object).GetMethod("Equals", null); - // If the classlib doesn't have Object.Equals/Object.GetHashCode, we don't need this. - if (_objectEqualsMethod == null && _objectGetHashCodeMethod == null) + // If the classlib doesn't have Object.Equals, we don't need this. + if (_objectEqualsMethod == null) return false; // Byref-like valuetypes cannot be boxed. @@ -85,17 +81,24 @@ private bool CanCompareValueTypeBits(MetadataType type) if (type.ContainsGCPointers) return false; - // TODO: what we're shooting for is overlapping fields - // or gaps between fields - if (type.IsExplicitLayout || type.GetClassLayout().Size != 0) + if (type.IsGenericDefinition) return false; + OverlappingFieldTracker overlappingFieldTracker = new OverlappingFieldTracker(type); + bool result = true; foreach (var field in type.GetFields()) { if (field.IsStatic) continue; + if (!overlappingFieldTracker.TrackField(field)) + { + // This field overlaps with another field - can't compare memory + result = false; + break; + } + TypeDesc fieldType = field.FieldType; if (fieldType.IsPrimitive || fieldType.IsEnum || fieldType.IsPointer || fieldType.IsFunctionPointer) { @@ -107,18 +110,13 @@ private bool CanCompareValueTypeBits(MetadataType type) break; } } - else if (fieldType.IsSignatureVariable) - { - return false; - } else { // Would be a suprise if this wasn't a valuetype. We checked ContainsGCPointers above. Debug.Assert(fieldType.IsValueType); - // If the field overrides Equals/GetHashCode, we can't use the fast helper because we need to call the method. - if (fieldType.FindVirtualFunctionTargetMethodOnObjectType(_objectEqualsMethod) != null || - fieldType.FindVirtualFunctionTargetMethodOnObjectType(_objectGetHashCodeMethod) != null) + // If the field overrides Equals, we can't use the fast helper because we need to call the method. + if (fieldType.FindVirtualFunctionTargetMethodOnObjectType(_objectEqualsMethod).OwningType == type) { result = false; break; @@ -132,7 +130,60 @@ private bool CanCompareValueTypeBits(MetadataType type) } } + // If there are gaps, we can't memcompare + if (result && overlappingFieldTracker.HasGaps) + result = false; + return result; } + + private struct OverlappingFieldTracker + { + private bool[] _usedBytes; + + public OverlappingFieldTracker(MetadataType type) + { + _usedBytes = new bool[type.InstanceFieldSize.AsInt]; + } + + public bool TrackField(FieldDesc field) + { + int fieldBegin = field.Offset.AsInt; + + TypeDesc fieldType = field.FieldType; + + int fieldEnd; + if (fieldType.IsPointer || fieldType.IsFunctionPointer) + { + fieldEnd = fieldBegin + field.Context.Target.PointerSize; + } + else + { + Debug.Assert(fieldType.IsValueType); + fieldEnd = fieldBegin + ((DefType)fieldType).InstanceFieldSize.AsInt; + } + + for (int i = fieldBegin; i < fieldEnd; i++) + { + if (_usedBytes[i]) + return false; + _usedBytes[i] = true; + } + + return true; + } + + public bool HasGaps + { + get + { + for (int i = 0; i < _usedBytes.Length; i++) + if (!_usedBytes[i]) + return true; + + return false; + } + } + } } }