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

Get rid of EETypePtr #97207

Merged
merged 3 commits into from
Jan 21, 2024
Merged

Get rid of EETypePtr #97207

merged 3 commits into from
Jan 21, 2024

Conversation

MichalStrehovsky
Copy link
Member

Resolves dotnet/runtimelab#232.

This wrapper served two purposes over MethodTable*:

  • Make sure equality semantics were enforced at the time where we could have multiple MethodTable* representing the same type. This is no longer the case and we don't need it for this purpose.
  • Save us from typing unsafe

Apart from this, it also pessimized codegen (see the linked issue). Bye EETypePtr.

I didn't fully delete it because the CorElementType conversion is still there. We can deal with that later.

Cc @dotnet/ilc-contrib

Resolves dotnet/runtimelab#232.

This wrapper served two purposes over `MethodTable*`:

* Make sure equality semantics were enforced at the time where we could have multiple `MethodTable*` representing the same type. This is no longer the case and we don't need it for this purpose.
* Save us from typing `unsafe`

Apart from this, it also pessimized codegen (see the linked issue). Bye `EETypePtr`.

I didn't fully delete it because the `CorElementType` conversion is still there. We can deal with that later.
@ghost
Copy link

ghost commented Jan 19, 2024

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Resolves dotnet/runtimelab#232.

This wrapper served two purposes over MethodTable*:

  • Make sure equality semantics were enforced at the time where we could have multiple MethodTable* representing the same type. This is no longer the case and we don't need it for this purpose.
  • Save us from typing unsafe

Apart from this, it also pessimized codegen (see the linked issue). Bye EETypePtr.

I didn't fully delete it because the CorElementType conversion is still there. We can deal with that later.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

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 once the CI is green. (It may be a good idea to run outer loop too.)

@jakobbotsch
Copy link
Member

Apart from this, it also pessimized codegen (see the linked issue).

I would be curious to learn about the concrete examples here. I'm aware of the tailcall issue, but the other one I'd be interested in looking closer at.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

Apart from this, it also pessimized codegen (see the linked issue).

I would be curious to learn about the concrete examples here. I'm aware of the tailcall issue, but the other one I'd be interested in looking closer at.

Can we get jit diffs for the PR or that doesn't work for non-JIT changes?

@MichalStrehovsky MichalStrehovsky merged commit 6fafbad into dotnet:main Jan 21, 2024
123 of 129 checks passed
@MichalStrehovsky MichalStrehovsky deleted the eetypeptr branch January 21, 2024 12:11
@jakobbotsch
Copy link
Member

Can we get jit diffs for the PR or that doesn't work for non-JIT changes?

Sadly I don't think we have an automated way to get diffs for a change like this one.

tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
Resolves dotnet/runtimelab#232.

This wrapper served two purposes over `MethodTable*`:

* Make sure equality semantics were enforced at the time where we could have multiple `MethodTable*` representing the same type. This is no longer the case and we don't need it for this purpose.
* Save us from typing `unsafe`

Apart from this, it also pessimized codegen (see the linked issue). Bye `EETypePtr`.

I didn't fully delete it because the `CorElementType` conversion is still there. We can deal with that later.
@kunalspathak
Copy link
Member

I am seeing crashes if I create an app using dotnet new console -aot and use IlcToolsPath to try local ilc/clrjit. It seems that there is a mismatch happening on shipped SDK that has EETypePtr and the locally built. Any idea how to fix this?

Repro steps:

  1. dotnet new console -aot
  2. Add IlcToolsPath, etc. pointing to the local build
  3. dotnet publish -c Release
  4. Run application results in segfault.
2431 0x0000555555619e10 in System.EETypePtr__EETypePtrOf<System___Canon> ()
#72432 0x00005555555c64d6 in String__CreateStringFromEncoding ()
#72433 0x00005555555f2ce4 in Internal.NativeFormat.NativeReader__DecodeString ()
#72434 0x00005555555f0db2 in Internal.Metadata.NativeFormat.MetadataReader__GetConstantStringValue ()
#72435 0x00005555555f0404 in System.Reflection.Runtime.TypeInfos.NativeFormat.NativeFormatRuntimeNamedTypeInfo__get_Name ()
#72436 0x00005555555f0006 in System.Reflection.Runtime.TypeInfos.RuntimeNamedTypeInfo__get_FullName ()
#72437 0x00005555555f02ff in System.Reflection.Runtime.TypeInfos.NativeFormat.NativeFormatRuntimeNamedTypeInfo__ToString ()
#72438 0x00005555555c5c25 in System.RuntimeType__ToString ()
#72439 0x00005555555c8c07 in System.Exception__AppendExceptionStackFrame ()
#72440 0x00005555555e9ebe in System.Runtime.EH__AppendExceptionStackFrameViaClasslib ()
#72441 0x00005555555ea276 in System.Runtime.EH__DispatchEx ()
#72442 0x00005555555ea12b in System.Runtime.EH__RhThrowEx ()
#72443 0x00005555555b52d3 in RhpThrowEx ()
#72444 0x0000555555619e10 in System.EETypePtr__EETypePtrOf<System___Canon> ()
#72445 0x00005555555c64d6 in String__CreateStringFromEncoding ()
#72446 0x00005555555c4018 in System.AppContext__InitializeDataStore ()
#72447 0x00005555555c4469 in System.AppContext___cctor ()
#72448 0x00005555555ec713 in System.Runtime.CompilerServices.ClassConstructorRunner__EnsureClassConstructorRun ()
#72449 0x00005555555ec5f9 in System.Runtime.CompilerServices.ClassConstructorRunner__CheckStaticClassConstructionReturnGCStaticBase ()
#72450 0x00005555555c4151 in System.AppContext__OnFirstChanceException ()
#72451 0x00005555555e9cad in System.Runtime.EH__OnFirstChanceExceptionViaClassLib ()
#72452 0x00005555555ea220 in System.Runtime.EH__DispatchEx ()
#72453 0x00005555555ea12b in System.Runtime.EH__RhThrowEx ()
#72454 0x00005555555b52d3 in RhpThrowEx ()
#72455 0x0000555555619e10 in System.EETypePtr__EETypePtrOf<System___Canon> ()
#72456 0x00005555555c64d6 in String__CreateStringFromEncoding ()
#72457 0x00005555555f4abd in Internal.Runtime.CompilerHelpers.StartupCodeHelpers__InitializeCommandLineArgs ()
#72458 0x000055555560c3c0 in nativeaot__Module___StartupCodeMain ()
#72459 0x00007ffff7c6c083 in __libc_start_main (main=0x55555555a900 <main>, argc=1, argv=0x7fffffffdb88, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffdb78) at ../csu/libc-start.c:308
#72460 0x0000555555559c9e in _start ()

Reverting this change fixes this crash.

@jkotas
Copy link
Member

jkotas commented Jan 24, 2024

Publish your app using locally built packages: https://github.com/dotnet/runtime/blob/main/docs/workflow/building/coreclr/nativeaot.md#building-packages . This will guarantee that you have matching set of bits.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate EETypePtr overhead
4 participants