Remove static variable holding unboxing stubs by davidwrighton · Pull Request #38838 · dotnet/runtime

davidwrighton · 2020-07-07T00:38:08Z

UnboxingMethodDesc is a type that is not intended to escape JIT
interface
This fix is a combination of 2 changes.
- First, a change for creation of ldtoken to a generic valuetype method
- Second, moving a bool to indicate whether or not a method is an
  unboxing stub into the MethodWithToken data structure instead of
  passing it as a side parameter everywhere
Also, a test case for various generic ldtoken cases is added to
crossgen2smoke
And, minor fix to make single method compilation of a generic method
function correctly. (There was a typo)

Fix issue #35531

- UnboxingMethodDesc is a type that is not intended to escape JIT interface - This fix is a combination of 2 changes. - First, a change for creation of ldtoken to a generic valuetype method - Second, moving a bool to indicate whether or not a method is an unboxing stub into the MethodWithToken data structure instead of passing it as a side parameter everywhere - Also, a test case for various generic ldtoken cases is added to crossgen2smoke - And, minor fix to make single method compilation of a generic method function correctly. (There was a typo)

MichalStrehovsky

Thank you for cleaning it up!

MichalStrehovsky · 2020-07-07T07:18:18Z


 private ISymbolNode CreateMethodHandleHelper(MethodWithToken method)
 {
- bool useUnboxingStub = method.Method.IsUnboxingThunk();


After this is gone, does this file still need using Internal.JitInterface;?

janvorli

LGTM, I like making handling the unboxing stub closer to runtime / CG1. I remember I have wondered why was it so different when making a change in a nearby area in the past.

janvorli · 2020-07-07T11:33:56Z

+ // AND
+ // The methods are on the same type
+ Debug.Assert((methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||
+ (!methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||


The first two terms in the assert condition can be simplified to just single !owningMethod.HasInstantiation

janvorli · 2020-07-07T11:39:39Z

 public string SingleMethodTypeName { get; set; }
 public string SingleMethodName { get; set; }
- public string[] SingleMethodGenericArgs { get; set; }
+ public string[] SingleMethodGenericArg { get; set; }


I wonder why have you changed the plural to singular when it is an array of arguments?

The name must match the name of the argument or our command line parser just drops the argument and does nothing with it. I decided I didn't want to change the external argument.

davidwrighton · 2020-07-07T00:38:08Z

UnboxingMethodDesc is a type that is not intended to escape JIT
interface
This fix is a combination of 2 changes.
- First, a change for creation of ldtoken to a generic valuetype method
- Second, moving a bool to indicate whether or not a method is an
  unboxing stub into the MethodWithToken data structure instead of
  passing it as a side parameter everywhere
Also, a test case for various generic ldtoken cases is added to
crossgen2smoke
And, minor fix to make single method compilation of a generic method
function correctly. (There was a typo)

Fix issue #35531

- UnboxingMethodDesc is a type that is not intended to escape JIT interface - This fix is a combination of 2 changes. - First, a change for creation of ldtoken to a generic valuetype method - Second, moving a bool to indicate whether or not a method is an unboxing stub into the MethodWithToken data structure instead of passing it as a side parameter everywhere - Also, a test case for various generic ldtoken cases is added to crossgen2smoke - And, minor fix to make single method compilation of a generic method function correctly. (There was a typo)

MichalStrehovsky

Thank you for cleaning it up!

MichalStrehovsky · 2020-07-07T07:18:18Z


 private ISymbolNode CreateMethodHandleHelper(MethodWithToken method)
 {
- bool useUnboxingStub = method.Method.IsUnboxingThunk();


After this is gone, does this file still need using Internal.JitInterface;?

janvorli

LGTM, I like making handling the unboxing stub closer to runtime / CG1. I remember I have wondered why was it so different when making a change in a nearby area in the past.

janvorli · 2020-07-07T11:33:56Z

+ // AND
+ // The methods are on the same type
+ Debug.Assert((methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||
+ (!methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||


The first two terms in the assert condition can be simplified to just single !owningMethod.HasInstantiation

janvorli · 2020-07-07T11:39:39Z

 public string SingleMethodTypeName { get; set; }
 public string SingleMethodName { get; set; }
- public string[] SingleMethodGenericArgs { get; set; }
+ public string[] SingleMethodGenericArg { get; set; }


I wonder why have you changed the plural to singular when it is an array of arguments?

The name must match the name of the argument or our command line parser just drops the argument and does nothing with it. I decided I didn't want to change the external argument.

davidwrighton · 2020-07-07T00:38:08Z

UnboxingMethodDesc is a type that is not intended to escape JIT
interface
This fix is a combination of 2 changes.
- First, a change for creation of ldtoken to a generic valuetype method
- Second, moving a bool to indicate whether or not a method is an
  unboxing stub into the MethodWithToken data structure instead of
  passing it as a side parameter everywhere
Also, a test case for various generic ldtoken cases is added to
crossgen2smoke
And, minor fix to make single method compilation of a generic method
function correctly. (There was a typo)

Fix issue #35531

- UnboxingMethodDesc is a type that is not intended to escape JIT interface - This fix is a combination of 2 changes. - First, a change for creation of ldtoken to a generic valuetype method - Second, moving a bool to indicate whether or not a method is an unboxing stub into the MethodWithToken data structure instead of passing it as a side parameter everywhere - Also, a test case for various generic ldtoken cases is added to crossgen2smoke - And, minor fix to make single method compilation of a generic method function correctly. (There was a typo)

MichalStrehovsky

Thank you for cleaning it up!

MichalStrehovsky · 2020-07-07T07:18:18Z


 private ISymbolNode CreateMethodHandleHelper(MethodWithToken method)
 {
- bool useUnboxingStub = method.Method.IsUnboxingThunk();


After this is gone, does this file still need using Internal.JitInterface;?

janvorli

LGTM, I like making handling the unboxing stub closer to runtime / CG1. I remember I have wondered why was it so different when making a change in a nearby area in the past.

janvorli · 2020-07-07T11:33:56Z

+ // AND
+ // The methods are on the same type
+ Debug.Assert((methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||
+ (!methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||


The first two terms in the assert condition can be simplified to just single !owningMethod.HasInstantiation

janvorli · 2020-07-07T11:39:39Z

 public string SingleMethodTypeName { get; set; }
 public string SingleMethodName { get; set; }
- public string[] SingleMethodGenericArgs { get; set; }
+ public string[] SingleMethodGenericArg { get; set; }


I wonder why have you changed the plural to singular when it is an array of arguments?

The name must match the name of the argument or our command line parser just drops the argument and does nothing with it. I decided I didn't want to change the external argument.

davidwrighton · 2020-07-07T00:38:08Z

UnboxingMethodDesc is a type that is not intended to escape JIT
interface
This fix is a combination of 2 changes.
- First, a change for creation of ldtoken to a generic valuetype method
- Second, moving a bool to indicate whether or not a method is an
  unboxing stub into the MethodWithToken data structure instead of
  passing it as a side parameter everywhere
Also, a test case for various generic ldtoken cases is added to
crossgen2smoke
And, minor fix to make single method compilation of a generic method
function correctly. (There was a typo)

Fix issue #35531

- UnboxingMethodDesc is a type that is not intended to escape JIT interface - This fix is a combination of 2 changes. - First, a change for creation of ldtoken to a generic valuetype method - Second, moving a bool to indicate whether or not a method is an unboxing stub into the MethodWithToken data structure instead of passing it as a side parameter everywhere - Also, a test case for various generic ldtoken cases is added to crossgen2smoke - And, minor fix to make single method compilation of a generic method function correctly. (There was a typo)

MichalStrehovsky

Thank you for cleaning it up!

MichalStrehovsky · 2020-07-07T07:18:18Z


 private ISymbolNode CreateMethodHandleHelper(MethodWithToken method)
 {
- bool useUnboxingStub = method.Method.IsUnboxingThunk();


After this is gone, does this file still need using Internal.JitInterface;?

janvorli

LGTM, I like making handling the unboxing stub closer to runtime / CG1. I remember I have wondered why was it so different when making a change in a nearby area in the past.

janvorli · 2020-07-07T11:33:56Z

+ // AND
+ // The methods are on the same type
+ Debug.Assert((methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||
+ (!methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||


The first two terms in the assert condition can be simplified to just single !owningMethod.HasInstantiation

janvorli · 2020-07-07T11:39:39Z

 public string SingleMethodTypeName { get; set; }
 public string SingleMethodName { get; set; }
- public string[] SingleMethodGenericArgs { get; set; }
+ public string[] SingleMethodGenericArg { get; set; }


I wonder why have you changed the plural to singular when it is an array of arguments?

The name must match the name of the argument or our command line parser just drops the argument and does nothing with it. I decided I didn't want to change the external argument.

davidwrighton · 2020-07-07T00:38:08Z

UnboxingMethodDesc is a type that is not intended to escape JIT
interface
This fix is a combination of 2 changes.
- First, a change for creation of ldtoken to a generic valuetype method
- Second, moving a bool to indicate whether or not a method is an
  unboxing stub into the MethodWithToken data structure instead of
  passing it as a side parameter everywhere
Also, a test case for various generic ldtoken cases is added to
crossgen2smoke
And, minor fix to make single method compilation of a generic method
function correctly. (There was a typo)

Fix issue #35531

- UnboxingMethodDesc is a type that is not intended to escape JIT interface - This fix is a combination of 2 changes. - First, a change for creation of ldtoken to a generic valuetype method - Second, moving a bool to indicate whether or not a method is an unboxing stub into the MethodWithToken data structure instead of passing it as a side parameter everywhere - Also, a test case for various generic ldtoken cases is added to crossgen2smoke - And, minor fix to make single method compilation of a generic method function correctly. (There was a typo)

MichalStrehovsky

Thank you for cleaning it up!

MichalStrehovsky · 2020-07-07T07:18:18Z


 private ISymbolNode CreateMethodHandleHelper(MethodWithToken method)
 {
- bool useUnboxingStub = method.Method.IsUnboxingThunk();


After this is gone, does this file still need using Internal.JitInterface;?

janvorli

LGTM, I like making handling the unboxing stub closer to runtime / CG1. I remember I have wondered why was it so different when making a change in a nearby area in the past.

janvorli · 2020-07-07T11:33:56Z

+ // AND
+ // The methods are on the same type
+ Debug.Assert((methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||
+ (!methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||


The first two terms in the assert condition can be simplified to just single !owningMethod.HasInstantiation

janvorli · 2020-07-07T11:39:39Z

 public string SingleMethodTypeName { get; set; }
 public string SingleMethodName { get; set; }
- public string[] SingleMethodGenericArgs { get; set; }
+ public string[] SingleMethodGenericArg { get; set; }


I wonder why have you changed the plural to singular when it is an array of arguments?

The name must match the name of the argument or our command line parser just drops the argument and does nothing with it. I decided I didn't want to change the external argument.

davidwrighton · 2020-07-07T00:38:08Z

UnboxingMethodDesc is a type that is not intended to escape JIT
interface
This fix is a combination of 2 changes.
- First, a change for creation of ldtoken to a generic valuetype method
- Second, moving a bool to indicate whether or not a method is an
  unboxing stub into the MethodWithToken data structure instead of
  passing it as a side parameter everywhere
Also, a test case for various generic ldtoken cases is added to
crossgen2smoke
And, minor fix to make single method compilation of a generic method
function correctly. (There was a typo)

Fix issue #35531

- UnboxingMethodDesc is a type that is not intended to escape JIT interface - This fix is a combination of 2 changes. - First, a change for creation of ldtoken to a generic valuetype method - Second, moving a bool to indicate whether or not a method is an unboxing stub into the MethodWithToken data structure instead of passing it as a side parameter everywhere - Also, a test case for various generic ldtoken cases is added to crossgen2smoke - And, minor fix to make single method compilation of a generic method function correctly. (There was a typo)

MichalStrehovsky

Thank you for cleaning it up!

MichalStrehovsky · 2020-07-07T07:18:18Z


 private ISymbolNode CreateMethodHandleHelper(MethodWithToken method)
 {
- bool useUnboxingStub = method.Method.IsUnboxingThunk();


After this is gone, does this file still need using Internal.JitInterface;?

janvorli

LGTM, I like making handling the unboxing stub closer to runtime / CG1. I remember I have wondered why was it so different when making a change in a nearby area in the past.

janvorli · 2020-07-07T11:33:56Z

+ // AND
+ // The methods are on the same type
+ Debug.Assert((methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||
+ (!methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||


The first two terms in the assert condition can be simplified to just single !owningMethod.HasInstantiation

janvorli · 2020-07-07T11:39:39Z

 public string SingleMethodTypeName { get; set; }
 public string SingleMethodName { get; set; }
- public string[] SingleMethodGenericArgs { get; set; }
+ public string[] SingleMethodGenericArg { get; set; }


I wonder why have you changed the plural to singular when it is an array of arguments?

The name must match the name of the argument or our command line parser just drops the argument and does nothing with it. I decided I didn't want to change the external argument.

davidwrighton · 2020-07-07T00:38:08Z

UnboxingMethodDesc is a type that is not intended to escape JIT
interface
This fix is a combination of 2 changes.
- First, a change for creation of ldtoken to a generic valuetype method
- Second, moving a bool to indicate whether or not a method is an
  unboxing stub into the MethodWithToken data structure instead of
  passing it as a side parameter everywhere
Also, a test case for various generic ldtoken cases is added to
crossgen2smoke
And, minor fix to make single method compilation of a generic method
function correctly. (There was a typo)

Fix issue #35531

- UnboxingMethodDesc is a type that is not intended to escape JIT interface - This fix is a combination of 2 changes. - First, a change for creation of ldtoken to a generic valuetype method - Second, moving a bool to indicate whether or not a method is an unboxing stub into the MethodWithToken data structure instead of passing it as a side parameter everywhere - Also, a test case for various generic ldtoken cases is added to crossgen2smoke - And, minor fix to make single method compilation of a generic method function correctly. (There was a typo)

MichalStrehovsky

Thank you for cleaning it up!

MichalStrehovsky · 2020-07-07T07:18:18Z


 private ISymbolNode CreateMethodHandleHelper(MethodWithToken method)
 {
- bool useUnboxingStub = method.Method.IsUnboxingThunk();


After this is gone, does this file still need using Internal.JitInterface;?

janvorli

LGTM, I like making handling the unboxing stub closer to runtime / CG1. I remember I have wondered why was it so different when making a change in a nearby area in the past.

janvorli · 2020-07-07T11:33:56Z

+ // AND
+ // The methods are on the same type
+ Debug.Assert((methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||
+ (!methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||


The first two terms in the assert condition can be simplified to just single !owningMethod.HasInstantiation

janvorli · 2020-07-07T11:39:39Z

 public string SingleMethodTypeName { get; set; }
 public string SingleMethodName { get; set; }
- public string[] SingleMethodGenericArgs { get; set; }
+ public string[] SingleMethodGenericArg { get; set; }


I wonder why have you changed the plural to singular when it is an array of arguments?

The name must match the name of the argument or our command line parser just drops the argument and does nothing with it. I decided I didn't want to change the external argument.

davidwrighton · 2020-07-07T00:38:08Z

UnboxingMethodDesc is a type that is not intended to escape JIT
interface
This fix is a combination of 2 changes.
- First, a change for creation of ldtoken to a generic valuetype method
- Second, moving a bool to indicate whether or not a method is an
  unboxing stub into the MethodWithToken data structure instead of
  passing it as a side parameter everywhere
Also, a test case for various generic ldtoken cases is added to
crossgen2smoke
And, minor fix to make single method compilation of a generic method
function correctly. (There was a typo)

Fix issue #35531

- UnboxingMethodDesc is a type that is not intended to escape JIT interface - This fix is a combination of 2 changes. - First, a change for creation of ldtoken to a generic valuetype method - Second, moving a bool to indicate whether or not a method is an unboxing stub into the MethodWithToken data structure instead of passing it as a side parameter everywhere - Also, a test case for various generic ldtoken cases is added to crossgen2smoke - And, minor fix to make single method compilation of a generic method function correctly. (There was a typo)

MichalStrehovsky

Thank you for cleaning it up!

MichalStrehovsky · 2020-07-07T07:18:18Z


 private ISymbolNode CreateMethodHandleHelper(MethodWithToken method)
 {
- bool useUnboxingStub = method.Method.IsUnboxingThunk();


After this is gone, does this file still need using Internal.JitInterface;?

janvorli

LGTM, I like making handling the unboxing stub closer to runtime / CG1. I remember I have wondered why was it so different when making a change in a nearby area in the past.

janvorli · 2020-07-07T11:33:56Z

+ // AND
+ // The methods are on the same type
+ Debug.Assert((methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||
+ (!methodContext.HasInstantiation && !owningMethod.HasInstantiation) ||


The first two terms in the assert condition can be simplified to just single !owningMethod.HasInstantiation

janvorli · 2020-07-07T11:39:39Z

 public string SingleMethodTypeName { get; set; }
 public string SingleMethodName { get; set; }
- public string[] SingleMethodGenericArgs { get; set; }
+ public string[] SingleMethodGenericArg { get; set; }


I wonder why have you changed the plural to singular when it is an array of arguments?

The name must match the name of the argument or our command line parser just drops the argument and does nothing with it. I decided I didn't want to change the external argument.

davidwrighton requested review from MichalStrehovsky and janvorli July 7, 2020 00:38

Dotnet-GitSync-Bot added the area-ReadyToRun label Jul 7, 2020

davidwrighton linked an issue Jul 7, 2020 that may be closed by this pull request
Make unboxing MethodDescs local to CorInfoImpl #35531
Closed

MichalStrehovsky approved these changes Jul 7, 2020
View reviewed changes

janvorli approved these changes Jul 7, 2020
View reviewed changes

davidwrighton merged commit 55f006f into dotnet:masterJul 7, 2020

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

davidwrighton deleted the issue_35531 branch April 20, 2021 17:42