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.

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

Removing Entry2MethodDesc as it is unnecessary #111756

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented Jan 23, 2025

It appears that the pathways that Entry2MethodDesc has for virtual resolution are unnecessary. Testing has shown that everything seems to work without this logic, and removing excess baggage is always good, and a bit less code should run a bit faster. As a bonus, this simplifies the logic around adding support for cached interface dispatch in CoreCLR.

Also, fix the crossgen2 test legs, as they are currently broken due to issues with the now NativeAOT'd crossgen2 binary.

Copy link
Contributor

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

@davidwrighton
Copy link
Member Author

/azp run runtime-coreclr crossgen2 outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidwrighton davidwrighton changed the title Experimental change removing Entry2MethodDesc to see what breaks Removing Entry2MethodDesc as it is unnecessary Jan 24, 2025
@davidwrighton davidwrighton marked this pull request as ready for review January 24, 2025 17:38
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 24, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 24, 2025
@davidwrighton
Copy link
Member Author

Analysis of the crossgen2 outerloop failures indicates they are not related to this change

@@ -679,7 +679,8 @@
<CrossgenCmd Condition="'$(__CompositeBuildMode)' == ''">$(CrossgenCmd) --crossgen2-parallelism 1</CrossgenCmd>

<CrossgenCmd>$(CrossgenCmd) --verify-type-and-field-layout</CrossgenCmd>
<CrossgenCmd>$(CrossgenCmd) --crossgen2-path "$(__BinDir)\$(BuildArchitecture)\crossgen2\crossgen2.dll"</CrossgenCmd>
<CrossgenCmd Condition="'$(RunningOnUnix)' == 'true'">$(CrossgenCmd) --crossgen2-path "$(__BinDir)\$(BuildArchitecture)\crossgen2\crossgen2"</CrossgenCmd>
Copy link
Member

Choose a reason for hiding this comment

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

Can't the same path work on Windows? Removing the .exe.

@AaronRobinsonMSFT
Copy link
Member

It looks like this change means there is no way for a Delegate to capture a "virtual" function and force a look-up at runtime. I can't conjure the correct C# at the moment but conceptually it would look like the following:

class A
{
    public virtual void M() {}
}

class B : A
{
    public override void M() {}
}

Delegate d = ...; // Create delegate for A::M but using a B instance.

@jkotas
Copy link
Member

jkotas commented Jan 27, 2025

It looks like this change means there is no way for a Delegate to capture a "virtual" function and force a look-up at runtime.

There is a way to do that - such delegates have to be created via reflection:

public class A { public virtual void M() { } }

typeof(A).GetMethod("M").CreateDelegate<Action<A>>() <- this delegate captures a "virtual" function

These delegates are special cased before calling Entry2MethodDesc. Entry2MethodDesc is never called on the VSD stub that they point to.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -679,7 +679,8 @@
<CrossgenCmd Condition="'$(__CompositeBuildMode)' == ''">$(CrossgenCmd) --crossgen2-parallelism 1</CrossgenCmd>

<CrossgenCmd>$(CrossgenCmd) --verify-type-and-field-layout</CrossgenCmd>
<CrossgenCmd>$(CrossgenCmd) --crossgen2-path "$(__BinDir)\$(BuildArchitecture)\crossgen2\crossgen2.dll"</CrossgenCmd>
<CrossgenCmd Condition="'$(RunningOnUnix)' == 'true'">$(CrossgenCmd) --crossgen2-path "$(__BinDir)\$(BuildArchitecture)\crossgen2\crossgen2"</CrossgenCmd>
<CrossgenCmd Condition="'$(RunningOnUnix)' != 'true'">$(CrossgenCmd) --crossgen2-path "$(__BinDir)\$(BuildArchitecture)\crossgen2\crossgen2.exe"</CrossgenCmd>
Copy link
Member

Choose a reason for hiding this comment

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

There is another instance of this problem at

value: $(productDirectory)$(dir)$(targetFlavor)$(dir)crossgen2$(dir)crossgen2.dll
- ${{ if ne(parameters.archType, 'x64') }}:
- name: crossgen2location
value: $(productDirectory)$(dir)$(targetFlavor)$(dir)x64$(dir)crossgen2$(dir)crossgen2.dll
that seems to be causing the remaining crossgen outer loop failures.

@davidwrighton davidwrighton merged commit 3ee4c41 into dotnet:main Jan 28, 2025
133 of 139 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Jan 28, 2025
* main: (31 commits)
  Fix linux-x86 build (dotnet#111861)
  Add FrozenDictionary specialization for integers / enums (dotnet#111886)
  [SRM] Refactor reading from streams. (dotnet#111323)
  Sign the DAC and DBI during the build process instead of in separate steps (dotnet#111416)
  Removing Entry2MethodDesc as it is unnecessary (dotnet#111756)
  Cross Product for Vector2 and Vector4 (dotnet#111265)
  Handle unicode in absolute URI path for combine. (dotnet#111710)
  Drop RequiresProcessIsolation on mcc tests (dotnet#111887)
  [main] Update dependencies from dotnet/roslyn (dotnet#111691)
  new trimmer feature System.TimeZoneInfo.Invariant (dotnet#111215)
  [browser] reduce msbuild memory footprint (dotnet#111751)
  Add debugging checks for stack overflow tests failure (dotnet#111867)
  Localized file check-in by OneLocBuild Task: Build definition ID 679: Build ID 2629821 (dotnet#111884)
  Bump main to preview2 (dotnet#111882)
  Avoid generic virtual dispatch for frozen collections alternate lookup (dotnet#108732)
  Bump main versioning to preview1 (dotnet#111880)
  Switch OneLoc to main (dotnet#111872)
  Improve docs on building ILVerify (dotnet#111851)
  Update Debian version to 13 (dotnet#111768)
  win32: add fallback to environment vars for system folder (dotnet#109673)
  ...
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.

3 participants