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.

Sign up for GitHub

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

Jump to bottom

Do not check object references for equality after hot reload #73009

Merged
merged 20 commits into from
Aug 8, 2022
Merged

Do not check object references for equality after hot reload #73009

merged 20 commits into from
Aug 8, 2022

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Jul 28, 2022

RuntimeType member's cache is purged after hot reload, therefore same member references are not equal after hotreload which causing issue like mentioned in Hot Reload breaks MethodInfo equality

Updated/added Object.Equals overloads to compare metadata token and module in case hot reload taken place instead of comparing object references.

Fixes #69427

@ghost ghost assigned buyaa-n Jul 28, 2022
@ghost
Copy link

ghost commented Jul 28, 2022

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

Issue Details

RuntimeType member's cache is purged after hot reload, therefore same member references are not equal after hotreload which causing issue like mentioned in Hot Reload breaks MethodInfo equality

Updated/added Object.Equals overloads to compare metadata token and module in case hot reload taken place instead of comparing object references.

Fixes #69427

Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection
Milestone: -

@runfoapp runfoapp bot mentioned this pull request Aug 1, 2022
Open
IDisposable
IDisposable reviewed Aug 2, 2022
View reviewed changes
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeEventInfo.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeFieldInfo.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.CoreCLR.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeParameterInfo.cs Outdated Show resolved Hide resolved
src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimePropertyInfo.cs Outdated Show resolved Hide resolved
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 4, 2022
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 4, 2022 •
edited
Loading

Do you understand why? Have you been able to create a small standalone repro using just the reflection APIs?

No, I could not understood why, a typical failure log looks like this:

Dynamic.Operator.Tests.PlusEqualTypeTests.Byte [FAIL]
System.ArgumentException : ParameterExpression of type 'System.String' cannot be used for delegate parameter of type 'System.Object'
        Stack Trace:
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(954,0): at System.Linq.Expressions.Expression.ValidateLambdaArgs(Type delegateType, Expres
  sion& body, ReadOnlyCollection`1 parameters, String paramName)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(717,0): at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, String na
  me, Boolean tailCall, IEnumerable`1 parameters)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Runtime\CompilerServices\CallSiteBinder.cs(192,0): at System.Runtime.CompilerServices.CallSiteBinder.Stitch[T](Expression bindi
  ng, LambdaSignature`1 signature)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Runtime\CompilerServices\CallSiteBinder.cs(132,0): at System.Runtime.CompilerServices.CallSiteBinder.BindCore[T](CallSite`1 sit
  e, Object[] args)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Dynamic\UpdateDelegates.Generated.cs(157,0): at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0
  arg0, T1 arg1)
          D:\dotnet\pr2\src\libraries\System.Dynamic.Runtime\tests\Dynamic.Context\Conformance.dynamic.context.operator.compound.type.PlusEqual.cs(76,0): at Dynamic.Operator.Tests.PlusEqualTypeTests.B
  yte()
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          D:\dotnet\pr2\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodInvoker.cs(69,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

just know reverting back the Equals logic fixes those failures, will dig more

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 4, 2022
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 4, 2022

Another log

System.Text.Tests.EncodingTests.GetEncoding_FromProvider_ByCodePage_WithDisallowedEncoding_Throws(encodingName: "utf-7", codePage: 65000) [FAIL]
        System.ArgumentException : ParameterExpression of type 'System.Text.EncodingProvider' cannot be used for delegate parameter of type 'System.Reflection.Binder'
        Stack Trace:
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(954,0): at System.Linq.Expressions.Expression.ValidateLambdaArgs(Type delegateType, Expres
  sion& body, ReadOnlyCollection`1 parameters, String paramName)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(717,0): at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, String na
  me, Boolean tailCall, IEnumerable`1 parameters)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(689,0): at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, Boolean t
  ailCall, IEnumerable`1 parameters)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(651,0): at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, Parameter
  Expression[] parameters)
          D:\dotnet\pr2\src\libraries\System.Runtime\tests\System\Text\EncodingTests.cs(43,0): at System.Text.Tests.EncodingTests.GetEncoding_FromProvider_ByCodePage_WithDisallowedEncoding_Throws(Stri
  ng encodingName, Int32 codePage)
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          D:\dotnet\pr2\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodInvoker.cs(64,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
        System.ArgumentException : ParameterExpression of type 'System.Text.EncodingProvider' cannot be used for delegate parameter of type 'System.Reflection.Binder'
        Stack Trace:
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(954,0): at System.Linq.Expressions.Expression.ValidateLambdaArgs(Type delegateType, Expres
  sion& body, ReadOnlyCollection`1 parameters, String paramName)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(717,0): at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, String na
  me, Boolean tailCall, IEnumerable`1 parameters)
      System.Text.Tests.EncodingTests.GetEncoding_FromProvider_ByEncodingName_WithDisallowedEncoding_Throws(encodingName: "utf-7", codePage: 65000) [FAIL]
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(689,0): at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, Boolean t
  ailCall, IEnumerable`1 parameters)
          D:\dotnet\pr2\src\libraries\System.Linq.Expressions\src\System\Linq\Expressions\LambdaExpression.cs(651,0): at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, Parameter
  Expression[] parameters)
          D:\dotnet\pr2\src\libraries\System.Runtime\tests\System\Text\EncodingTests.cs(71,0): at System.Text.Tests.EncodingTests.GetEncoding_FromProvider_ByEncodingName_WithDisallowedEncoding_Throws(
  String encodingName, Int32 codePage)
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          D:\dotnet\pr2\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodInvoker.cs(64,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

@jkotas
Copy link
Member

jkotas commented Aug 5, 2022

Looking at the test failures - the reflected type needs to be also included in Equals method.

@runfoapp runfoapp bot mentioned this pull request Aug 5, 2022
Closed
buyaa-n and others added 2 commits August 5, 2022 17:54
@buyaa-n
Add reflected type into Equals, check MetadataUpdater.IsSupported in …
eb9d197 
…GetHashCode
@jkotas
Perf tweaks for Equals and GetHashCode
72d21c6 
- No need to use GetHashCodeOfPtr when we are wrapping the hashcode using HashCode.Combine that takes care of the randomization
- Use underlying type handle for type hashcodes. It is faster than the default object hashcode.
- Delete special casing for MetadataUpdater.IsSupported from RuntimeMethodInfo.GetHashCode. It is actually a de-optimization for RuntimeMethodInfo with the two changes above, and about neutral for the rest.
- Avoid virtual call for ReflectedType comparison
- Restore comment for RuntimeMethodInfo
@jkotas
Copy link
Member

jkotas commented Aug 6, 2022 •
edited
Loading

@buyaa-n I have pushed a commit with a bunch of perf tweaks and simplifications. The commit description has the details. Let me know if it looks good to you or if you have any questions.

@buyaa-n @jkotas
Apply suggestions from code review
cbeb099 
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 6, 2022

@buyaa-n I have pushed a commit with a bunch of perf tweaks and simplifications. The commit description has the details. Let me know if it looks good to you or if you have any questions.

Thanks for the description, makes sense to me

@jkotas
Copy link
Member

jkotas commented Aug 8, 2022

Looks good to me, modulo the last two comments

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 8, 2022

Failures unrelated and known issue, merging

@buyaa-n buyaa-n merged commit 7e0f0b7 into dotnet:main Aug 8, 2022
@buyaa-n buyaa-n deleted the hot-reload branch August 8, 2022 17:57
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@steveharter steveharter steveharter left review comments

@jkotas jkotas jkotas approved these changes

@stephentoub stephentoub stephentoub left review comments

@sbomer sbomer sbomer left review comments

@IDisposable IDisposable IDisposable approved these changes

Assignees

@buyaa-n buyaa-n

Labels
area-System.Reflection
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hot Reload breaks MethodInfo equality
7 participants
@buyaa-n @jkotas @sbomer @IDisposable @stephentoub @steveharter @AaronRobinsonMSFT