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

Replace Mono.Cecil usage with modified S.R.MetadataLoadContext #1254

Open
jpobst opened this issue Sep 20, 2024 · 1 comment
Open

Replace Mono.Cecil usage with modified S.R.MetadataLoadContext #1254

jpobst opened this issue Sep 20, 2024 · 1 comment
Labels
proposal Issue raised for discussion, we do not even know if the change would be desirable yet

Comments

@jpobst
Copy link
Contributor

jpobst commented Sep 20, 2024

Replacing our usage of Cecil with System.Reflection.MetadataLoadContext is an interesting idea that should in theory result in faster build times for tasks that need to scan assemblies.

For example, looking for all top-level classes that inherit Java.Lang.Object in Mono.Android.dll:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
FindJavaTypes - Cecil 34.19 ms 0.673 ms 0.775 ms 1200.0000 1133.3333 800.0000 45.82 MB
FindJavaTypes - MLC 10.15 ms 0.198 ms 0.235 ms 234.3750 203.1250 109.3750 5.34 MB

However, while MLC performs great resolving assemblies and iterating types, its performance tanks once you start digging deeper, like calling GetNestedTypes () or GetMethods () on it. If we extend the previous test case to also check nested types, MLC is already unusable:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
FindJavaTypes - Cecil 40.39 ms 0.793 ms 1.583 ms 1230.7692 1153.8462 769.2308 47.6 MB
FindJavaTypes - MLC 695.74 ms 13.769 ms 14.140 ms 17000.0000 16000.0000 3000.0000 343.34 MB

Interestingly, this performance hit seems to be related to trying to shoehorn the results into a System.Reflection compatible API. If we bypass that and instead add a way to call the otherwise internal EcmaDefinitionType.GetNestedTypesCore (NameFilter?) method directly, performance is back to excellent:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
FindJavaTypes - Cecil 40.39 ms 0.793 ms 1.583 ms 1230.7692 1153.8462 769.2308 47.6 MB
FindJavaTypes - MLC+ 15.16 ms 0.292 ms 0.273 ms 328.1250 312.5000 125.0000 7.32 MB

If we create an MLC version of JavaCallableWrappers's CecilImporter class, we can see it performs worse, as it scans nested types, methods, constructors, etc.

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
JCW Importer - Cecil 820.3 ms 13.94 ms 12.36 ms 15000.0000 14000.0000 3000.0000 358.88 MB
JCW Importer - MLC 1,827.1 ms 19.77 ms 17.52 ms 47000.0000 31000.0000 3000.0000 1065.28 MB

Once again, as we create "backdoors" to internal enumeration methods for these objects, we start to see performance wins over Cecil:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
JCW Importer - Cecil 820.3 ms 13.94 ms 12.36 ms 15000.0000 14000.0000 3000.0000 358.88 MB
JCW Importer - MLC+ 546.5 ms 9.46 ms 8.85 ms 26000.0000 6000.0000 2000.0000 576.12 MB

MLC performance has been reported at dotnet/runtime#30886, but it does not seem like MLC is very actively under development anymore.

We could probably get some decent build performance wins if we created a fork of MLC that used the default assembly/module/type logic but had custom nested type/methods/constructors logic.

@jpobst jpobst added the proposal Issue raised for discussion, we do not even know if the change would be desirable yet label Sep 20, 2024
@jonpryor
Copy link
Member

A related question is where would we want to consider migrating away from Cecil.

We've elsewhere discussed the idea of using Java.Interop.Tools.JavaCallableWrappers from a linker step. The linker already uses Cecil, so for Java.Interop.Tools.JavaCallableWrappers/jcw-gen, I don't think we necessarily have a reason to migrate away from Cecil.

generator, on the other hand, probably could use System.MetadataLoadContext or a fork of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Issue raised for discussion, we do not even know if the change would be desirable yet
Projects
None yet
Development

No branches or pull requests

2 participants