Add managed blocking info for lock and monitor waits

[kouvel]

• Added a managed ThreadBlockingInfo struct that is similar to CoreCLR's DebugBlockingItem
• Updated Lock, Condition, and Monitor to record the info
• The LockOwnerOSThreadId and LockOwnerManagedThreadId properties can be used by debuggers to determine which thread owns a lock that the current thread is waiting on
  • In CoreCLR, Monitor records the info using the Monitor object kinds. In NativeAOT, Lock and Condition are used for Monitor waits, so the object kinds would be Lock/Condition. For now, Mono's Monitor does not record this info.

[dotnet-policy-service]

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

[hughbe]

Is there an issue/rationale for making these classes managed? Just curious

[kouvel]

Is there an issue/rationale for making these classes managed? Just curious

It's a bit easier to track this info in a managed surface when the types involved are managed (eg. Lock). It also unifies how the info is stored between runtimes, and makes it easier for other managed types to record similar info in the future.

[noahfalk]

LGTM. Have the VS folks confirmed they can work with this?

[noahfalk]

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

[kouvel]

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

[noahfalk]

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

runtime/src/coreclr/debug/di/rsthread.cpp

Line 2750 in d92ac1f

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;

.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

[kouvel]

Have the VS folks confirmed they can work with this?

Someone from their team has taken a look and confirmed that it looks ok for them to proceed with.

[kouvel]

• Added a managed ThreadBlockingInfo struct that is similar to CoreCLR's DebugBlockingItem
• Updated Lock, Condition, and Monitor to record the info
• The LockOwnerOSThreadId and LockOwnerManagedThreadId properties can be used by debuggers to determine which thread owns a lock that the current thread is waiting on
  • In CoreCLR, Monitor records the info using the Monitor object kinds. In NativeAOT, Lock and Condition are used for Monitor waits, so the object kinds would be Lock/Condition. For now, Mono's Monitor does not record this info.

[dotnet-policy-service]

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

[hughbe]

Is there an issue/rationale for making these classes managed? Just curious

[kouvel]

Is there an issue/rationale for making these classes managed? Just curious

It's a bit easier to track this info in a managed surface when the types involved are managed (eg. Lock). It also unifies how the info is stored between runtimes, and makes it easier for other managed types to record similar info in the future.

[noahfalk]

LGTM. Have the VS folks confirmed they can work with this?

[noahfalk]

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

[kouvel]

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

[noahfalk]

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

runtime/src/coreclr/debug/di/rsthread.cpp

Line 2750 in d92ac1f

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;

.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

[kouvel]

Have the VS folks confirmed they can work with this?

Someone from their team has taken a look and confirmed that it looks ok for them to proceed with.

[kouvel]

• Added a managed ThreadBlockingInfo struct that is similar to CoreCLR's DebugBlockingItem
• Updated Lock, Condition, and Monitor to record the info
• The LockOwnerOSThreadId and LockOwnerManagedThreadId properties can be used by debuggers to determine which thread owns a lock that the current thread is waiting on
  • In CoreCLR, Monitor records the info using the Monitor object kinds. In NativeAOT, Lock and Condition are used for Monitor waits, so the object kinds would be Lock/Condition. For now, Mono's Monitor does not record this info.

[dotnet-policy-service]

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

[hughbe]

Is there an issue/rationale for making these classes managed? Just curious

[kouvel]

Is there an issue/rationale for making these classes managed? Just curious

It's a bit easier to track this info in a managed surface when the types involved are managed (eg. Lock). It also unifies how the info is stored between runtimes, and makes it easier for other managed types to record similar info in the future.

[noahfalk]

LGTM. Have the VS folks confirmed they can work with this?

[noahfalk]

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

[kouvel]

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

[noahfalk]

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

runtime/src/coreclr/debug/di/rsthread.cpp

Line 2750 in d92ac1f

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;

.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

[kouvel]

Have the VS folks confirmed they can work with this?

Someone from their team has taken a look and confirmed that it looks ok for them to proceed with.

[kouvel]

• Added a managed ThreadBlockingInfo struct that is similar to CoreCLR's DebugBlockingItem
• Updated Lock, Condition, and Monitor to record the info
• The LockOwnerOSThreadId and LockOwnerManagedThreadId properties can be used by debuggers to determine which thread owns a lock that the current thread is waiting on
  • In CoreCLR, Monitor records the info using the Monitor object kinds. In NativeAOT, Lock and Condition are used for Monitor waits, so the object kinds would be Lock/Condition. For now, Mono's Monitor does not record this info.

[dotnet-policy-service]

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

[hughbe]

Is there an issue/rationale for making these classes managed? Just curious

[kouvel]

Is there an issue/rationale for making these classes managed? Just curious

It's a bit easier to track this info in a managed surface when the types involved are managed (eg. Lock). It also unifies how the info is stored between runtimes, and makes it easier for other managed types to record similar info in the future.

[noahfalk]

LGTM. Have the VS folks confirmed they can work with this?

[noahfalk]

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

[kouvel]

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

[noahfalk]

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

runtime/src/coreclr/debug/di/rsthread.cpp

Line 2750 in d92ac1f

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;

.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

[kouvel]

Have the VS folks confirmed they can work with this?

Someone from their team has taken a look and confirmed that it looks ok for them to proceed with.

[kouvel]

• Added a managed ThreadBlockingInfo struct that is similar to CoreCLR's DebugBlockingItem
• Updated Lock, Condition, and Monitor to record the info
• The LockOwnerOSThreadId and LockOwnerManagedThreadId properties can be used by debuggers to determine which thread owns a lock that the current thread is waiting on
  • In CoreCLR, Monitor records the info using the Monitor object kinds. In NativeAOT, Lock and Condition are used for Monitor waits, so the object kinds would be Lock/Condition. For now, Mono's Monitor does not record this info.

[dotnet-policy-service]

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

[hughbe]

Is there an issue/rationale for making these classes managed? Just curious

[kouvel]

Is there an issue/rationale for making these classes managed? Just curious

It's a bit easier to track this info in a managed surface when the types involved are managed (eg. Lock). It also unifies how the info is stored between runtimes, and makes it easier for other managed types to record similar info in the future.

[noahfalk]

LGTM. Have the VS folks confirmed they can work with this?

[noahfalk]

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

[kouvel]

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

[noahfalk]

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

runtime/src/coreclr/debug/di/rsthread.cpp

Line 2750 in d92ac1f

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;

.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

[kouvel]

Have the VS folks confirmed they can work with this?

Someone from their team has taken a look and confirmed that it looks ok for them to proceed with.

[kouvel]

• Added a managed ThreadBlockingInfo struct that is similar to CoreCLR's DebugBlockingItem
• Updated Lock, Condition, and Monitor to record the info
• The LockOwnerOSThreadId and LockOwnerManagedThreadId properties can be used by debuggers to determine which thread owns a lock that the current thread is waiting on
  • In CoreCLR, Monitor records the info using the Monitor object kinds. In NativeAOT, Lock and Condition are used for Monitor waits, so the object kinds would be Lock/Condition. For now, Mono's Monitor does not record this info.

[dotnet-policy-service]

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

[hughbe]

Is there an issue/rationale for making these classes managed? Just curious

[kouvel]

Is there an issue/rationale for making these classes managed? Just curious

It's a bit easier to track this info in a managed surface when the types involved are managed (eg. Lock). It also unifies how the info is stored between runtimes, and makes it easier for other managed types to record similar info in the future.

[noahfalk]

LGTM. Have the VS folks confirmed they can work with this?

[noahfalk]

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

[kouvel]

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

[noahfalk]

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

runtime/src/coreclr/debug/di/rsthread.cpp

Line 2750 in d92ac1f

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;

.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

[kouvel]

Have the VS folks confirmed they can work with this?

Someone from their team has taken a look and confirmed that it looks ok for them to proceed with.

[kouvel]

• Added a managed ThreadBlockingInfo struct that is similar to CoreCLR's DebugBlockingItem
• Updated Lock, Condition, and Monitor to record the info
• The LockOwnerOSThreadId and LockOwnerManagedThreadId properties can be used by debuggers to determine which thread owns a lock that the current thread is waiting on
  • In CoreCLR, Monitor records the info using the Monitor object kinds. In NativeAOT, Lock and Condition are used for Monitor waits, so the object kinds would be Lock/Condition. For now, Mono's Monitor does not record this info.

[dotnet-policy-service]

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

[hughbe]

Is there an issue/rationale for making these classes managed? Just curious

[kouvel]

Is there an issue/rationale for making these classes managed? Just curious

It's a bit easier to track this info in a managed surface when the types involved are managed (eg. Lock). It also unifies how the info is stored between runtimes, and makes it easier for other managed types to record similar info in the future.

[noahfalk]

LGTM. Have the VS folks confirmed they can work with this?

[noahfalk]

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

[kouvel]

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

[noahfalk]

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

runtime/src/coreclr/debug/di/rsthread.cpp

Line 2750 in d92ac1f

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;

.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

[kouvel]

Have the VS folks confirmed they can work with this?

Someone from their team has taken a look and confirmed that it looks ok for them to proceed with.

[kouvel]

• Added a managed ThreadBlockingInfo struct that is similar to CoreCLR's DebugBlockingItem
• Updated Lock, Condition, and Monitor to record the info
• The LockOwnerOSThreadId and LockOwnerManagedThreadId properties can be used by debuggers to determine which thread owns a lock that the current thread is waiting on
  • In CoreCLR, Monitor records the info using the Monitor object kinds. In NativeAOT, Lock and Condition are used for Monitor waits, so the object kinds would be Lock/Condition. For now, Mono's Monitor does not record this info.

[dotnet-policy-service]

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

[hughbe]

Is there an issue/rationale for making these classes managed? Just curious

[kouvel]

Is there an issue/rationale for making these classes managed? Just curious

It's a bit easier to track this info in a managed surface when the types involved are managed (eg. Lock). It also unifies how the info is stored between runtimes, and makes it easier for other managed types to record similar info in the future.

[noahfalk]

LGTM. Have the VS folks confirmed they can work with this?

[noahfalk]

Do we need a 2nd definition of these types? Could we update DebugBlockingItem and DebugBlockingItemType to match instead?

[kouvel]

The current definition appears to be used by debugger APIs to enumerate the blocking items. It seems there would need to be a second object on the stack for maintaining a separate linked list that contains info only about Monitor blocking, as the current APIs to get info from a blocking object would not work with Lock. I was thinking it might be easier to remove the current definitions in the future after debuggers switch to using the managed surface, and maybe deprecate some of the relevant debugger APIs. Thoughts?

[noahfalk]

The types that get exposed publicly should be CorDebugBlockingObject and CorDebugBlockingReason in the ICorDebug interface. The types exposed in the DAC IDacDbiInterface are not public and can be changed as needed. The code to convert from the internal to public types occurs here:

runtime/src/coreclr/debug/di/rsthread.cpp

Line 2750 in d92ac1f

SIZE_T dacObjIndex = dacBlockingObjects.Size()-i-1;

.

However if we removed the domain field off the internal type we'd have to repopulate it from somewhere else and it looks like we've never done the work to convert ICorDebug to understand that CoreCLR just has a single AppDomain. Its probably not that hard, but I wouldn't feel right asking you to do that refactoring when the ground work isn't there yet. So I'd say lets not worry about this, it was just a potential for some minor deduplication.

In terms of deprecation we've rarely ever done it in the past but it is possible if supporting the old APIs is ongoing hassle and we've allowed some time to pass so that the new API is easily available. I'd think next release at the earliest unless there is some pressing need. We'd need to remove the usage from our tests, remove code from the runtime, and put out notifications of the breaking change. Thanks Kount!

[kouvel]

Have the VS folks confirmed they can work with this?

Someone from their team has taken a look and confirmed that it looks ok for them to proceed with.