-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cDAC] Implement ReJIT portion of SOSDacImpl::GetMethodDescData #109936
Conversation
Tagging subscribers to this area: @tommcdon |
cbb0de3
to
b5f76de
Compare
02dafce
to
6610aa6
Compare
6610aa6
to
d4de7c9
Compare
....Diagnostics.DataContractReader.Abstractions/Contracts/Extensions/ICodeVersionsExtensions.cs
Outdated
Show resolved
Hide resolved
@@ -7,10 +7,20 @@ namespace Microsoft.Diagnostics.DataContractReader; | |||
|
|||
|
|||
[DebuggerDisplay("{Hex}")] | |||
public readonly struct TargetNUInt | |||
public readonly struct TargetNUInt : IEquatable<TargetNUInt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds IEquatable
to make comparing TargetNUInt
s less verbose.
|
||
kSuppressParams = 0x80000000 | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debated making this part of the data type, but didn't want to tie it in case the values change. Hiding them in the ReJIT
implementation means we need a second copy for testing.
bool IsEnabled() => throw new NotImplementedException(); | ||
|
||
RejitState GetRejitState(ILCodeVersionHandle codeVersionHandle) => throw new NotImplementedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the cdac I feel like we should either make an effort to make the data structures and algorithms match the runtime directly or make a conscious effort to have a higher level API that abstracts away some of the details. Putting GetRejitState
and GetRejitId
here doesn't feel like it accomplishes either of those tasks. In the runtime they are part of ILCodeVersion
and in the cdac they are now part of a different conceptual contract, but we don't change the abstraction level.
Curious to know what other people's thoughts are here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, we do need contract methods to get this metadata (id/state) as the cDAC is setup to not allow users to access the marshalled data structures directly. Enforcing the use of contract methods allows for versioning independent of runtime changes.
Another option would be to put these methods on the ICodeVersions
contract. However, I believe separating the ICodeVersions
contract from the IReJIT
contract is beneficial as it allows us to separate the logic determining what code version is active from associated metadata which ReJIT/TieredCompilation add to code versions. In case this metadata structure changes in the future it will be simple to change just the relevant contract implementation without modifying the larger code versioning logic.
When TieredCompilation SOS APIs are implemented, I think we should create an ITieredCompilation
contract that allows us to get the optimization tier metadata off of NativeCodeVersions
.
Since FEATURE_REJIT
can be enabled/disabled separately from FEATURE_CODE_VERSIONING
, I think this also gives us a path to adapt to toggleable runtime features through contract versions. If for example ReJIT is disabled in a target, the ReJIT contract can be different and let cDAC users know ReJIT is disabled.
I am also curious if other people have suggestions on better ways to implement the ReJIT cDAC functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might be two levels we are talking about here
- data structures and algorithms match the runtime: this seems like what we want for the data descriptors and the contract implementations in the cDAC reader. They are not directly exposed to consumers (for example, the ISOSDacInterface implementation in SOSDacImpl.cs).
- higher level API: this should be the contracts and the methods on those contracts
I do view the CodeVersions
and ReJIT
as separate at a higher level - one depends on the other, but I think that is a natural/expected part of these contracts. With the IL/NativeCodeVersionHandle
providing the opaque common link, separate CodeVersions
and ReJIT
contracts (and eventually TieredCompilation
) makes sense to me.
...ed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CodeVersions_1.cs
Outdated
Show resolved
Hide resolved
internal readonly TargetPointer Module; | ||
internal readonly uint MethodDefinition; | ||
internal readonly TargetPointer ILCodeVersionNode; | ||
internal ILCodeVersionHandle(TargetPointer module, uint methodDef, TargetPointer ilCodeVersionNodeAddress) | ||
{ | ||
if (module != TargetPointer.Null && ilCodeVersionNodeAddress != TargetPointer.Null) | ||
throw new ArgumentException("Both MethodDesc and ILCodeVersionNode cannot be non-null"); | ||
|
||
if (module != TargetPointer.Null && methodDef == 0) | ||
throw new ArgumentException("MethodDefinition must be non-zero if Module is non-null"); | ||
|
||
Module = module; | ||
MethodDefinition = methodDef; | ||
ILCodeVersionNode = ilCodeVersionNodeAddress; | ||
} | ||
public static ILCodeVersionHandle Invalid => new ILCodeVersionHandle(TargetPointer.Null, 0, TargetPointer.Null); | ||
public bool IsValid => Module != TargetPointer.Null || ILCodeVersionNode != TargetPointer.Null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on removing these details from the *CodeVersionHandle
in the contract doc? I see them more as opaque handles that get returned from / passed to different contracts, with the module / method definiton / node on them being implementation details that aren't part of the contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think we should have only the publicly usable parts of the handle. I'll make that change.
...ed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CodeVersions_1.cs
Outdated
Show resolved
Hide resolved
...ed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CodeVersions_1.cs
Outdated
Show resolved
Hide resolved
...ed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CodeVersions_1.cs
Outdated
Show resolved
Hide resolved
.../cdacreader/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs
Outdated
Show resolved
Hide resolved
c559f0a
to
541066d
Compare
.../cdacreader/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs
Outdated
Show resolved
Hide resolved
.../cdacreader/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs
Outdated
Show resolved
Hide resolved
...e/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ReJIT_1.cs
Outdated
Show resolved
Hide resolved
...e/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ReJIT_1.cs
Outdated
Show resolved
Hide resolved
/ba-g Build analysis blocked by #110395 |
…et#109936) * Modifies CodeVersions contract * Adds to ReJIT contract * Adds contract extension methods as helpers for when functionality can be implemented in terms of versioned APIs.
…et#109936) * Modifies CodeVersions contract * Adds to ReJIT contract * Adds contract extension methods as helpers for when functionality can be implemented in terms of versioned APIs.
Adds ReJIT support to
SOSDacImpl::GetMethodDescData
.Modifies ICodeVersions contract
GetActiveNativeCodeVersion
can be implemented in terms of existingICodeVersions
methodsGetActiveILCodeVersion
andGetActiveNativeCodeVersionForILCodeVersion
. With the goal of keeping the contract surface area as small as possible, I think it would be best to remove this method and implement as an extension method onICodeVersions
. Since it only relies on the existing contract APIs, the extension method does not need to be versioned.Adds to IReJIT contract
Adds fields to existing cDAC Data Types
FirstVersionNode
pointer toILCodeVersioningState. This is used to iterate the linked list of explicit
ILCodeVersionNodes`.Next
pointer toILCodeVersionNode
to iterate the linked list of explicit nodes.RejitState
uint
to ILCodeVersionNode`Testing
Tested manually using the ReJIT tooling in the coreclr rejit test. I ran the test in WinDBG and debugged WinDBG in VS to verify the SOS calls had the expected behavior.
Contributes to #109426