Skip to content

Comments

Add TypeName APIs to simplify metadata lookup.#111598

Merged
jkotas merged 37 commits intodotnet:mainfrom
teo-tsirpanis:typename-namespace-unescape
Feb 10, 2025
Merged

Add TypeName APIs to simplify metadata lookup.#111598
jkotas merged 37 commits intodotnet:mainfrom
teo-tsirpanis:typename-namespace-unescape

Conversation

@teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Jan 20, 2025 •
edited
Loading

Fixes #101774.

@ghost
Copy link

ghost commented Jan 20, 2025

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jan 20, 2025

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 20, 2025
@jkotas
Copy link
Member

jkotas commented Jan 20, 2025

Could you please delete

runtime/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

Lines 1492 to 1494 in 29013d8

<Compile Include="$(CommonPath)System\Reflection\Metadata\TypeNameHelpers.cs">
<Link>Common\System\Reflection\Metadata\TypeNameHelpers.cs</Link>
</Compile>
and replace the calls to it with this API?

(Other uses of TypeNameHelpers.cs should be delete too once these APIs are available in the LKG runtime.)

@jkotas jkotas requested a review from adamsitnik January 20, 2025 03:02
This was referenced Jan 20, 2025
Open
Open
Closed
@teo-tsirpanis teo-tsirpanis requested a review from Copilot January 20, 2025 22:15
Copilot AI reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

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.

Copilot reviewed 5 out of 17 changed files in this pull request and generated no comments.

Files not reviewed (12)
  • src/coreclr/tools/ILVerification/ILVerification.projitems: Language not supported
  • src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj: Language not supported
  • src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj: Language not supported
  • src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems: Language not supported
  • src/tools/illink/src/linker/Mono.Linker.csproj: Language not supported
  • src/libraries/Common/src/System/Reflection/Metadata/TypeNameHelpers.cs: Evaluated as low risk
  • src/coreclr/tools/Common/TypeSystem/Common/Utilities/CustomAttributeTypeNameParser.cs: Evaluated as low risk
  • src/tools/illink/src/linker/Linker/TypeNameResolver.cs: Evaluated as low risk
  • src/mono/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.Mono.cs: Evaluated as low risk
  • src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs: Evaluated as low risk
  • src/coreclr/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.CoreCLR.cs: Evaluated as low risk
  • src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.NativeAot.cs: Evaluated as low risk

@teo-tsirpanis teo-tsirpanis force-pushed the typename-namespace-unescape branch from e1a9730 to d08be6b Compare January 20, 2025 22:19
@teo-tsirpanis teo-tsirpanis force-pushed the typename-namespace-unescape branch from d08be6b to 1e3f969 Compare January 20, 2025 22:30
@build-analysis build-analysis bot mentioned this pull request Feb 6, 2025
Open
3 tasks
@adamsitnik adamsitnik mentioned this pull request Feb 6, 2025
Closed
@teo-tsirpanis
Use alternative strategy to provide a (message, typeName) overload …
b924a6f 
…in Mono, while still avoiding changes to the Mono runtime code.
@teo-tsirpanis teo-tsirpanis force-pushed the typename-namespace-unescape branch from 87cf79d to b924a6f Compare February 8, 2025 16:06
jkotas
jkotas reviewed Feb 9, 2025
View reviewed changes
jkotas
jkotas reviewed Feb 9, 2025
View reviewed changes
This was referenced Feb 9, 2025
Open
Closed
This was referenced Feb 10, 2025
Closed
Closed
@jkotas jkotas merged commit 617f9ee into dotnet:main Feb 10, 2025
138 of 143 checks passed
@teo-tsirpanis teo-tsirpanis deleted the typename-namespace-unescape branch February 10, 2025 22:20
@jkotas jkotas mentioned this pull request Feb 10, 2025
Closed
@jkotas
Copy link
Member

jkotas commented Feb 10, 2025

I have opened #112376 as a follow up to use these APIs in more places.

@teo-tsirpanis teo-tsirpanis added this to the 10.0.0 milestone Feb 11, 2025
grendello added a commit to grendello/runtime that referenced this pull request Feb 11, 2025
@grendello
Merge branch 'main' into dev/grendel/android-clr-host-local
35c42cf 
* main:
  Code clean up in AP for NonNull* (dotnet#112027)
  JIT: Invalidate LSRA's DFS tree if we aren't running new layout phase (dotnet#112364)
  Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250204.2 (dotnet#112339)
  Add doc on OS onboarding (dotnet#112026)
  Add `TypeName` APIs to simplify metadata lookup. (dotnet#111598)
  Internal monitor impl not using coop mutex causing deadlocks on Android. (dotnet#112358)
  Do not run NAOT arm64 OSX testing on all PRs (dotnet#112342)
  Special-case empty enumerables in AsyncEnumerable (dotnet#112321)
  Have mono handle ConvertToIntegerNative for Double and Single (dotnet#112206)
  Update dependencies from https://github.com/dotnet/arcade build 20250206.4 (dotnet#112338)
  System.Configuration.ConfigurationManager.Tests: use Assembly.Location to determine ThisApplicationPath. (dotnet#112231)
  Force write of local file header when "version needed to extract" changes (dotnet#112032)
  JIT: Don't reorder handler blocks (dotnet#112292)
  [RISC-V] Synthesize some floating constants inline (dotnet#111529)
  Enable `SA1000`: Spacing around keywords (dotnet#112302)
  Fix relocs for linux-riscv64 AOT (dotnet#112331)
@jkurdek jkurdek mentioned this pull request Feb 20, 2025
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2025
@lewing
Copy link
Member

lewing commented Jul 27, 2025

very high wasm regressions https://github.com/dotnet/performance/blob/main/reports/net9to10/linux-wasm-tiger_regression_report.md#2-617f9ee5f3---add-typename-apis-to-simplify-metadata-lookup-111598

@adamsitnik
Copy link
Member

adamsitnik commented Jul 28, 2025

very high wasm regressions https://github.com/dotnet/performance/blob/main/reports/net9to10/linux-wasm-tiger_regression_report.md#2-617f9ee5f3---add-typename-apis-to-simplify-metadata-lookup-111598

It's strange, this regression is WASM specific:

Windows:

{1EC066C6-FC16-4C00-A071-BA9FA0E999BC}

Linux:

{F8A1F4A1-F7C6-4BF2-93DD-3C8A4689DDF3}

If we zoom in, CLR has actually improved performance in that time range:

{1E150473-BEFF-4DDE-9715-7B2EA00BEF53}

@lewing I am 100% occupied with AI-related projects and I won't be able to help, but please report a new issue (it's very easy to miss GH notification from an old PR) and assign it to 10.0 milestone so we don't regress type loading for WASM in 10

@jkotas
Copy link
Member

jkotas commented Jul 28, 2025

It's strange, this regression is WASM specific

I expect that the regression is Mono interpreter specific.

This change https://github.com/dotnet/runtime/pull/111598/files#diff-02398900754074bfbfe1f634cd44c490e92f433d7cdd8eb217effde64b3581baR49 switched Type.GetType(string) to call common type parsing code to avoid bugs and behavior differences in Mono C type parsing code. The common type parsing code is written in C# that is slow when executed with Mono interpreter.

If we care about this regression, we can revert the change in Type.Mono.cs to fix it. It will re-introduce bugs and behavior differences on Mono that will need to be dealt with.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes

Copilot code review Copilot Copilot left review comments

@adamsitnik adamsitnik adamsitnik approved these changes

@steveisok steveisok Awaiting requested review from steveisok steveisok is a code owner

@vitek-karas vitek-karas Awaiting requested review from vitek-karas vitek-karas is a code owner

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees

@adamsitnik adamsitnik

Labels

area-System.Reflection.Metadata community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Simplify lookup of parsed TypeName in metadata

4 participants

@teo-tsirpanis @jkotas @lewing @adamsitnik