Skip to content

Use no-op MD Importer for DIA-backed stacktraces#129866

Merged
hoyosjs merged 19 commits into
dotnet:mainfrom
hoyosjs:juhoyosa/md-wrapper-noop-import
Jul 13, 2026
Merged

Use no-op MD Importer for DIA-backed stacktraces#129866
hoyosjs merged 19 commits into
dotnet:mainfrom
hoyosjs:juhoyosa/md-wrapper-noop-import

Conversation

@hoyosjs

@hoyosjs hoyosjs commented Jun 25, 2026

Copy link
Copy Markdown
Member

When resolving line numbers and sequence points for classic PDB-backed modules, Module::GetISymUnmanagedReader on main passes a real metadata importer into DIA. Materializing that importer can switch metadata access to the RW-backed variant, and later metadata reads can contend on its lock-heavy path.

This change avoids that transition by passing a process-wide no-op importer (NoopMetadataImport) that only satisfies the binder's non-null COM contract. Since it contains no module-linked state, it is a singleton and has no collectible-lifetime coupling.

NoopMetadataImport behavior:

  • QI supports IUnknown, IMetaDataImport, IMetaDataImport2 and does not expose IID_IGetIMDInternalImport.
  • AddRef/Release are process-lifetime no-ops.
  • Metadata APIs assert in Checked/Debug and return E_NOTIMPL in Release.

The binder requires a non-null metadata import interface, but the StackTrace/source-line in-proc path does not need metadata signatures from that importer. A no-op importer keeps the binder contract while avoiding the RW swap.

Performance tests

Same repro from issue: https://gist.github.com/jkotas/617109b64d9b0ad52b491cdf021b2f8a

Machine:

  • 6 physical cores / 12 logical processors

Experiment:

  • 2x2x2 matrix: baseline vs change, full vs portable PDB, PDB present vs deleted
  • 5 repetitions per cell
  • 20 seconds per repetition
  • Throughput metric: exceptions/sec in the harness

Results (avg exceptions/sec):

  • full + PDB present: 865 -> 855 (0.99x)
  • portable + PDB present: 14,115 -> 14,264 (1.01x)
  • portable + no PDB: 35,843 -> 36,545 (1.02x)
  • full + no PDB: 10,848 -> 26,381 (2.43x)

This matches the target scenario (full + no PDB) and keeps non-target cells effectively unchanged.

Fixes #90563

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR changes CoreCLR’s symbol-reader setup in Module::GetISymUnmanagedReader to avoid materializing a module’s real public metadata importer when constructing DIA-backed symbol readers, by instead passing a process-wide inert IMetaDataImport2 implementation.

Changes:

  • Introduces NoopMetadataImport, a singleton IMetaDataImport2 implementation intended only to satisfy the binder’s non-null metadata-import parameter.
  • Updates Module::GetISymUnmanagedReader to pass the noop importer to ISymUnmanagedBinder::GetReaderFromStream / GetReaderForFile instead of using the module’s importer.
  • Wires the new files into the VM build via src/coreclr/vm/CMakeLists.txt.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/coreclr/vm/noopmetadataimport.h Declares NoopMetadataImport singleton implementing IMetaDataImport2 as an inert binder placeholder.
src/coreclr/vm/noopmetadataimport.cpp Implements singleton creation, COM QI/AddRef/Release behavior, and E_NOTIMPL stubs for metadata APIs.
src/coreclr/vm/CMakeLists.txt Adds the new noop importer source/header to VM build lists.
src/coreclr/vm/ceeload.cpp Switches symbol-reader binder calls to use NoopMetadataImport::GetInstance() instead of module metadata importers.

Comment thread src/coreclr/vm/noopmetadataimport.h Outdated
Comment thread src/coreclr/vm/noopmetadataimport.cpp Outdated
Comment thread src/coreclr/vm/noopmetadataimport.h
Comment thread src/coreclr/vm/noopmetadataimport.cpp Outdated
Comment thread src/coreclr/vm/noopmetadataimport.cpp Outdated
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
Copilot AI review requested due to automatic review settings June 25, 2026 23:28
@hoyosjs hoyosjs enabled auto-merge (squash) June 25, 2026 23:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread src/coreclr/vm/noopmetadataimport.cpp Outdated
Comment thread src/coreclr/vm/ceeload.cpp Outdated
Comment thread src/coreclr/vm/ceeload.cpp Outdated
Comment thread src/coreclr/vm/noopmetadataimport.cpp Outdated
Comment thread src/coreclr/inc/corpriv.h Outdated
Comment thread src/coreclr/md/runtime/noopmetadataimport.cpp
Comment thread src/coreclr/vm/noopmetadataimport.cpp Outdated
Comment thread src/coreclr/vm/noopmetadataimport.cpp Outdated
Copilot AI review requested due to automatic review settings June 28, 2026 06:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread src/coreclr/md/runtime/noopmetadataimport.cpp
Comment thread src/coreclr/md/runtime/noopmetadataimport.cpp Outdated
Comment thread src/coreclr/vm/ceeload.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment thread src/coreclr/md/runtime/noopmetadataimport.cpp
Comment thread src/tests/Interop/IJW/NativeCallingManaged/NativeCallingManaged.cs
Comment thread src/tests/Interop/IJW/NativeCallingManaged/NativeCallingManaged.cs
@jkotas

jkotas commented Jul 2, 2026

Copy link
Copy Markdown
Member

fullpdbstacktrace is failing on CoreCLR, and the tests need to be disabled on Mono

Copilot AI review requested due to automatic review settings July 8, 2026 01:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comment thread src/tests/Interop/IJW/NativeCallingManaged/NativeCallingManaged.cs
Copilot AI review requested due to automatic review settings July 8, 2026 21:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comment thread src/tests/Interop/IJW/NativeCallingManaged/NativeCallingManaged.cs
Comment thread src/tests/Interop/IJW/NativeCallingManaged/NativeCallingManaged.cs
Copilot AI review requested due to automatic review settings July 10, 2026 18:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Comment thread src/tests/Common/helixpublishwitharcade.proj
Comment thread src/tests/Interop/IJW/NativeVarargs/NativeVarargsTest.cs
Comment thread src/tests/Interop/IJW/NativeCallingManaged/NativeCallingManaged.cs
@hoyosjs

hoyosjs commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

/ba-g fix for wasm is merged in main

@hoyosjs hoyosjs merged commit f98c7ad into dotnet:main Jul 13, 2026
115 of 118 checks passed
@hoyosjs hoyosjs deleted the juhoyosa/md-wrapper-noop-import branch July 13, 2026 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lock contention in StackTrace/Exception.ToString()

5 participants