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

[cdac] add v2 ExecutionManager contract for NibbleMap change #1

Open
wants to merge 7 commits into
base: nibble-optimize
Choose a base branch
from
Open

[cdac] add v2 ExecutionManager contract for NibbleMap change #1

wants to merge 7 commits into from

Conversation

max-charlamb
Copy link
Owner

@max-charlamb max-charlamb commented Nov 7, 2024 •
edited
Loading

Depends on: dotnet#108939

Changes

  • Bumps ExecutionManager contract version to 2
  • Refactors ExecutionManager related files into folder.
  • Factors out nearly all of ExecutionManager_1 to ExecutionManagerBase
  • Adds ExecutionManager_2
  • Runs tests for both versions of contract.

@max-charlamb max-charlamb marked this pull request as ready for review November 7, 2024 18:24
@max-charlamb max-charlamb changed the title add ExecutionManager contract version [cdac] add v2 ExecutionManager contract for NibbleMap change Nov 7, 2024
AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Nov 7, 2024
View reviewed changes
...gnostics.DataContractReader.Contracts/Contracts/ExecutionManager/Helpers/NibbleMapHelpers.cs Outdated Show resolved Hide resolved
...gnostics.DataContractReader.Contracts/Contracts/ExecutionManager/Helpers/NibbleMapHelpers.cs Outdated Show resolved Hide resolved
...gnostics.DataContractReader.Contracts/Contracts/ExecutionManager/Helpers/NibbleMapHelpers.cs Outdated Show resolved Hide resolved
...gnostics.DataContractReader.Contracts/Contracts/ExecutionManager/Helpers/NibbleMapHelpers.cs Outdated Show resolved Hide resolved
...gnostics.DataContractReader.Contracts/Contracts/ExecutionManager/Helpers/NibbleMapHelpers.cs Outdated Show resolved Hide resolved
src/native/managed/cdacreader/tests/ExecutionManagerTests/NibbleMapTestBuilder.cs Outdated Show resolved Hide resolved
src/native/managed/cdacreader/tests/ExecutionManagerTests/NibbleMapTestBuilder.cs Outdated Show resolved Hide resolved
src/native/managed/cdacreader/tests/ExecutionManagerTests/NibbleMapTestBuilder.cs Outdated Show resolved Hide resolved
src/native/managed/cdacreader/tests/ExecutionManagerTests/NibbleMapTestBuilder.cs Outdated Show resolved Hide resolved
src/native/managed/cdacreader/tests/ExecutionManagerTests/NibbleMapTestBuilder.cs Outdated Show resolved Hide resolved
elinor-fung
elinor-fung reviewed Nov 8, 2024
View reviewed changes
Copy link

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you also able to test out the E2E combined with your runtime nibble map change? Well, the part of the E2E that we have working - for JIT-ed methods, the method names should display correctly for something like !clrstack (should go through the ExecutionManager and the nibble map in order to get the method corresponding to the IPs).

src/native/managed/cdacreader/tests/ExecutionManagerTests/ExecutionManagerTests.cs Outdated Show resolved Hide resolved
...ft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManager_1.cs Outdated Show resolved Hide resolved
...ft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManager_2.cs Outdated Show resolved Hide resolved
...t.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/Helpers/NibbleMap_2.cs Outdated Show resolved Hide resolved
...t.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/Helpers/NibbleMap_2.cs Outdated

namespace Microsoft.Diagnostics.DataContractReader.ExecutionManagerHelpers;

// CoreCLR nibblemap with O(1) lookup time.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update docs/design/datacontracts/ExecutionManager.md to add version 2 with information about this nibble map?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some docs and an example. I'm not sure the best way to add docs for the second version. I added a section below explaining the differences and the new algorithm.

...agnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManagerFactory.cs Outdated Show resolved Hide resolved
@max-charlamb
Copy link
Owner Author

Were you also able to test out the E2E combined with your runtime nibble map change? Well, the part of the E2E that we have working - for JIT-ed methods, the method names should display correctly for something like !clrstack (should go through the ExecutionManager and the nibble map in order to get the method corresponding to the IPs).

Yes, I used WinDbg and debugged that with VS to verify that !sos clrstack has the correct stack (function names) and that the new nibblemap is invoked.

@max-charlamb max-charlamb mentioned this pull request Nov 8, 2024
Merged
@max-charlamb max-charlamb mentioned this pull request Nov 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elinor-fung elinor-fung elinor-fung left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT Awaiting requested review from AaronRobinsonMSFT

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@max-charlamb @AaronRobinsonMSFT @elinor-fung