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

Add DNMD into src/native #107961

Draft
wants to merge 148 commits into
base: main
Choose a base branch
from
Draft

Add DNMD into src/native #107961

wants to merge 148 commits into from
+29,397 −206

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Sep 17, 2024 •
edited
Loading

Add dnmd into the dotnet/runtime repo for usage by cDAC (and in the future, usage as the native ECMA-335 metadata library for the repo as a whole).

Depends on #107889, #108999

@AaronRobinsonMSFT
Initial commit of DNMD
4f1bc36 
Static lib for reading ECMA-335 metadata
Confirmed works against all .NET 6.0 BCL
assemblies.
@AaronRobinsonMSFT
Simple mddump impl
fdb4920 
Update dnmd.h for compiling in C++
@AaronRobinsonMSFT
Remove uses of C file I/O API
aaa8aac
@AaronRobinsonMSFT
Create table/column index enum
428b569 
Add a debugging process for validating
using the correct enumeration when using
an enumeration column index.
@AaronRobinsonMSFT
Add support for Portable PDBs
157318e 
Portable PDB support can be enabled by building
with the DNMD_PORTABLE_PDB define.
@AaronRobinsonMSFT
Minor nits
39670eb
@AaronRobinsonMSFT
Fix C and C++ issues on clang
fe7a626 
Still fails to compile on non-WIndows due to
missing PE image data structures.
@AaronRobinsonMSFT
Add PE data structures to platform.h
7c2f48a
@AaronRobinsonMSFT
Move around platform headers
87cfbbe 
Udate public API and remove CorTokenType
@AaronRobinsonMSFT
Incorrect macro used.
23382a8
@AaronRobinsonMSFT
Cursor updates
097796f
@AaronRobinsonMSFT
Change query API to permit bulk
eeb50cb 
operations.
@AaronRobinsonMSFT
Start implementation for interfaces
f5b058a 
Builds with no obvious regressions
in the mddump scenario.
@AaronRobinsonMSFT
Add build of static and shared
8a1a4d2 
library for dnmd_interfaces
@AaronRobinsonMSFT
Start Regression testing suite
d1abd88
@AaronRobinsonMSFT
Missed test props file.
302aaf1
@AaronRobinsonMSFT
Create CoTaskMemAlloc and malloc smart pointers
c11b1fd 
Implemented minimal IMetaDataDispenser
@AaronRobinsonMSFT
EnumTypeDefs always skips the first row.
10fe9f2
@AaronRobinsonMSFT
Add find logic for sorted tables in dnmd
6647a1f 
Testing for several methods on IMetaDataImport
@AaronRobinsonMSFT
Add impl and test for EnumMembers
ec235b3 
Add test for ResetEnum
Update HCORENUM implementation to be
  a singly linked list to reduce allocations.
Remove md_row_distance()
@AaronRobinsonMSFT
EnumParams and EnumFields
44f7f5a
@AaronRobinsonMSFT
EnumProperties
8e6ad50
@AaronRobinsonMSFT
Add entries for tables not defined in ECMA
5f1848e
@AaronRobinsonMSFT
Enum Events and UserStrings
018cf61 
Change low level UserStrings API
Add walk UserStrings heap API
@AaronRobinsonMSFT
GetClassLayout
a5ee998
@AaronRobinsonMSFT
Mark MetadataImportRO final and all methods
78b76ea 
as override
@AaronRobinsonMSFT
Nits
f3f5167
@AaronRobinsonMSFT
GetTypeSpecFromToken and GetRVA
bad5bf6
@AaronRobinsonMSFT
Add flags for specific table to mddump.
8f7c0de
@AaronRobinsonMSFT
IsGlobal
aa1db66 
Use "userstring" instead of "wchar" for APIs.
New API for reverse mapping from range to
  root in mapping table.
@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Sep 21, 2024 •
edited
Loading

for usage by cDAC

Is it actually going to be a net win to use unmanaged metadata reader in cDAC?

For cDAC in particular, the idea was to use DNMD for the IMetaDataImport exposure from CLRDataModule.

usage as the native ECMA-335 metadata library for the repo as a whole

It would make more sense to me to focus on this part: prove that this metadata reader is at least as good as the existing runtime reader and replace the current the runtime reader.

I'd like to eventually use DNMD throughout the repo, but I plan to do more perf work in actual CoreCLR and Mono scenarios (after merging this preferably as doing actual testing in the runtime will require some corresponding runtime changes).

@jkotas
Copy link
Member

jkotas commented Sep 21, 2024

For cDAC in particular, the idea was to use DNMD for the IMetaDataImport exposure from CLRDataModule.

Is IMetaDataImport interface exposed by this? I only see several custom enumerators.

Also, we have discussed the possibility of cDac world no longer supporting some of the redundant DAC interfaces like IXCLRData... that are only used by windbg for historic reasons and where fixing windbg may be a better long-term path.

@jkoritzinsky
Copy link
Member Author

It is through a "poor-man's COM Aggregation" implementation in the QueryInterface implementation.

@davidwrighton asked about using DNMD for this case (which is why I looked at doing this for the hackathon now).

This was referenced Sep 24, 2024
Open
Open
Open
@jkoritzinsky
Copy link
Member Author

Coming back to this: With some changes to DNMD, I was able to match (and seemingly beat) the performance of the current reader in CoreCLR in my local testing for read-only scenarios.

@jkoritzinsky jkoritzinsky mentioned this pull request Oct 17, 2024
Merged
@jkoritzinsky
Copy link
Member Author

I re-written the history and cleaned up this PR to not add as many commits to the history and utilize already-merged work from various PRs (including the perf improvements added to DNMD from experimenting with CoreCLR).

@jkoritzinsky jkoritzinsky force-pushed the dnmd-in-runtime branch 2 times, most recently from c03b575 to 32bf1b2 Compare October 31, 2024 23:19
This was referenced Nov 1, 2024
Open
Open
@jkoritzinsky jkoritzinsky force-pushed the dnmd-in-runtime branch 3 times, most recently from 15f5936 to b698b77 Compare November 1, 2024 20:35
@jkoritzinsky
Copy link
Member Author

/azp run runtime

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkoritzinsky
Integrate DNMD into the runtime repos build.
4167a4c 
Convert testing harness to C++11.
Convert DNMD PALs to use the minipal and the COM minipal.
Integrate DNMD testing into the CLR_Tools_Tests job.
@jkoritzinsky
Remove the implicit conversion from span<T> to T* and instead introdu…
6d99e39 
…ce a .data() member, matching the C++20 design for span<T>
This was referenced Nov 6, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkotas jkotas jkotas left review comments

@am11 am11 am11 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@jkoritzinsky jkoritzinsky

Labels
area-Infrastructure Hackathon Issues picked for Hackathon NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it
Projects
Runtime Infra
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jkoritzinsky @am11 @jkotas @AaronRobinsonMSFT