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

Don't report dependency on canonical forms just because there's GVMs #103331

Closed
wants to merge 3 commits into from

Conversation

MichalStrehovsky
Copy link
Member

#103039 (comment) found that we're generating some orphaned MethodTables. They seem to be coming from here. Wondering if anything breaks if we just delete this.

dotnet#103039 (comment) found that we're generating some orphaned `MethodTable`s. They seem to be coming from here. Wondering if anything breaks if we just delete this.
Copy link
Contributor

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

@filipnavara
Copy link
Member

Process terminated. Failed to create generic virtual method implementation


Declaring type: Generics+TestSimpleGVMScenarios+GenDerived`1<System.String>
Method name: GMethod1
Instantiation:
Argument 00000000: System.Int32


at System.RuntimeExceptionHelpers.FailFast(String, Exception, String, RhFailFastReason, IntPtr, IntPtr) + 0x250
at Internal.Runtime.TypeLoader.TypeLoaderEnvironment.ResolveGenericVirtualMethodTarget(RuntimeTypeHandle, RuntimeMethodHandle) + 0x310
at System.Runtime.TypeLoaderExports.<>c.<GVMLookupForSlotSlow>b__8_0(IntPtr, IntPtr, Object, IntPtr&) + 0x50
at System.Runtime.TypeLoaderExports.CacheMiss(IntPtr, IntPtr, RuntimeObjectFactory, Object) + 0x38
at System.Runtime.TypeLoaderExports.GVMLookupForSlotSlow(Object, RuntimeMethodHandle) + 0x68
at System.Runtime.TypeLoaderExports.GVMLookupForSlot(Object, RuntimeMethodHandle) + 0xbc
at Generics.TestSimpleGVMScenarios.[T](https://dev.azure.com/dnceng-public/public/_build/results?buildId=704798&view=ms.vss-test-web.build-test-results-tab&runId=17572496&resultId=100019&paneView=debug)estWithGenClassT + 0x38
at Generics.TestSimpleGVMScenarios.Run() + 0x1d8
at Generics.Run() + 0x58
at Program.<<Main>$>g__RunTest|0_0(Func`1, String) + 0x48
at Program.<Main>$(String[] args) + 0xd8

(keeping it here for posterity, the CI results will expire at some point)

@MichalStrehovsky
Copy link
Member Author

So there are some savings with this, but nothing to write home about:

Size statistics

Pull request #103331

Project Size before Size after Difference
avalonia.app-linux 24514576 24514576 0
avalonia.app-windows 22029312 22029312 0
hello-linux 1356512 1356512 0
hello-minimal-linux 1094280 1094280 0
hello-minimal-windows 863744 863744 0
hello-windows 1107456 1107456 0
kestrel-minimal-linux 5771328 5771328 0
kestrel-minimal-windows 5070848 5069824 -1024
reflection-linux 2232176 2232176 0
reflection-windows 1886208 1886208 0
webapiaot-linux 10424016 10419920 -4096
webapiaot-windows 9353216 9351680 -1536

I don't know if this is worth the extra 40 lines of code. @dotnet/ilc-contrib anyone thinks this is worth it? (I would lean towards "not worth it".)

@jkotas
Copy link
Member

jkotas commented Jun 13, 2024

Does this make us 100% clean wrt not generating any orphaned symbols? If it does, I would say that it is worth it.

@MichalStrehovsky
Copy link
Member Author

Does this make us 100% clean wrt not generating any orphaned symbols? If it does, I would say that it is worth it.

@ivanpovazan was #103039 (comment) the full list of symbols linker was able to strip, or just a sample?

@ivanpovazan
Copy link
Member

That was just a sample from SmokeTests/UnitTests project

@MichalStrehovsky
Copy link
Member Author

UnitTest is pretty representative (it's a kitchen sink).

It might be more valuable if we can not just get clean, but also stay clean. Maybe I could look if the object writer could assert that all generated symbols are also referenced, or are specified as explicit exports (e.g. UnmanagedCallersOnly entrypoints)?

@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 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.

4 participants