Skip to content

Partial revert of "Delete LOADEDMODULES cache", additional cleanup #116374

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

Merged
merged 3 commits into from
Jun 7, 2025

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Jun 6, 2025

Adding back implementation of IMetaDataImport::ResolveTypeRef to avoid breaking profilers. The break was detected by internal profiler tests.

Also deleted unnecessary IClassFactory implementation.

@Copilot Copilot AI review requested due to automatic review settings June 6, 2025 13:40
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 6, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

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 partially reverts the deletion of the LOADEDMODULES cache and reintroduces the implementation of IMetaDataImport::ResolveTypeRef to avoid breaking profiler tests. Key changes include:

  • Re-adding caching support via RegMeta::AddToCache and restoring ResolveTypeRef in regmeta_vm.cpp.
  • Removing obsolete COM class factory code and related functions.
  • Updating build scripts to include the new VM caching definitions and remove deprecated sources.

Reviewed Changes

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

Show a summary per file
File Description
src/coreclr/md/enc/mdinternalrw.cpp Added call to AddToCache to update cache after interface setup.
src/coreclr/md/compiler/regmeta_vm.cpp Restored AddToCache and ResolveTypeRef implementations.
src/coreclr/md/compiler/regmeta.cpp Removed old Release implementation and updated QueryInterface.
src/coreclr/md/compiler/disp.cpp Added calls to AddToCache in various scope creation routines.
Various CMakeLists.txt and header files Removed obsolete class factory code and added FEATURE_METADATA_IN_VM definition.
Comments suppressed due to low confidence (1)

src/coreclr/md/CMakeLists.txt:4

  • Ensure that the addition of FEATURE_METADATA_IN_VM is compatible with all dependent modules and that its behavior is clearly documented in related developer guides.
add_compile_definitions($<$<NOT:$<OR:$<BOOL:$<TARGET_PROPERTY:DAC_COMPONENT>>,$<BOOL:$<TARGET_PROPERTY:DBI_COMPONENT>>>>:FEATURE_METADATA_IN_VM>)

@jkotas jkotas requested a review from tommcdon June 6, 2025 13:42
@jkotas jkotas added area-VM-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 6, 2025
Copy link
Contributor

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

Copy link
Member

@tommcdon tommcdon left a comment

Choose a reason for hiding this comment

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

Thank you! The changes look good!

@jkotas jkotas force-pushed the profiler-revert branch from 1a06e95 to 54b824c Compare June 7, 2025 15:19
@jkotas
Copy link
Member Author

jkotas commented Jun 7, 2025

/ba-g timeout

@jkotas jkotas merged commit e9957dc into dotnet:main Jun 7, 2025
111 of 113 checks passed
@jkotas jkotas deleted the profiler-revert branch June 7, 2025 19:44
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.

2 participants