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

[Xamarin.Android.Build.Tasks] use System.Reflection.Metadata in <ResolveAssemblies/> #2612

Merged
merged 2 commits into from
Jan 16, 2019

Conversation

jonathanpeppers
Copy link
Member

Context: https://github.com/dotnet/corefx/tree/master/src/System.Reflection.Metadata/src/System/Reflection/Metadata
Context: https://github.com/jonathanpeppers/Benchmarks

There is a new System.Reflection.Metadata library from corefx for
reading .NET assemblies. It is a bit more performant than Mono.Cecil
because it is a different library with different opinions.

Some notes about System.Reflection.Metadata:

  • SRM has a forward "reader" style API
  • SRM uses lots of structs, and you have to do an additional call to
    lookup strings generally.
  • SRM, as far as I have seen, doesn't have APIs to modify and write
    out new assemblies.
  • SRM only supports "portable" pdb files.
  • SRM is not well documented yet. To discover usage, I read source
    code and/or unit tests.

From my benchmark above, it seems that SRM is 10x faster on
Windows/.NET framework and 5x faster on macOS/Mono.

So it makes sense for use to use SRM when reading assemblies (and we
don't need symbols), and continue with Mono.Cecil for the linker and
other things that modify assemblies.

There are a few places we can take advantage of SRM, but the simplest
with a reasonable impact was ResolveAssemblies:

Before:
320 ms  ResolveAssemblies                          1 calls
After:
112 ms  ResolveAssemblies                          1 calls

So a ~200ms savings on this MSBuild task, which runs on every build.
This was the Xamarin.Forms test project in this repo: a build with no
changes.

Changes

  • Added a MetadataResolver type, as a way to cache PEReader
    instances. This is a comparable drop-in replacement for
    DirectoryAssemblyResolver.
  • MonoAndroidHelper.IsReferenceAssembly now uses
    System.Reflection.Metadata instead of Mono.Cecil. This is used
    in a few other MSBuild tasks.
  • A MetadataExtensions provides an extension method to simplify
    getting the full name of a custom attribute. We can add more here as
    needed.

The resulting code should be the same, except we are using SRM over
Mono.Cecil.

Downstream

We will need to add the following assemblies to the installer:

  • System.Reflection.Metadata.dll
  • System.Collections.Immutable.dll

@jonathanpeppers
Copy link
Member Author

FYI some other places I tried using System.Reflection.Metadata:

  • ResolveLibraryProjectImports - only saved about 20ms. Since it only looks at EmbeddedResource, not much savings. I might still send a PR for that one, but it required enabling /unsafe on the project as well...
  • jcwgen in Java.Interop - this started to be a big undertaking. Then I hit the blocker where SRM only supports portable PDB files for symbols. I'm not sure we can use SRM for finding symbol information such as this. I could get the file name of a method, but only if the assembly had a portable PDB file.

Log files for build times above: logs.zip

@jonpryor
Copy link
Member

The macOS PR Release Build job failed:

Android/Xamarin.Android.Common.targets(1928,2): error : Exception while loading assemblies: System.MissingMethodException: Method not found: System.Reflection.AssemblyName System.Reflection.Metadata.AssemblyDefinition.GetAssemblyName()

This feels like we're somehow using the "wrong" System.Reflection.Metadata.dll file?

@jonathanpeppers
Copy link
Member Author

macOS was failing because xabuild.exe has its own System.Reflection.Metadata.dll (part of MSBuild).

We'll see how this goes.

…lveAssemblies/>

Context: https://github.com/dotnet/corefx/tree/master/src/System.Reflection.Metadata/src/System/Reflection/Metadata
Context: https://github.com/jonathanpeppers/Benchmarks

There is a new System.Reflection.Metadata library from corefx for
reading .NET assemblies. It is a bit more performant than Mono.Cecil
because it is a different library with different opinions.

Some notes about System.Reflection.Metadata:

- SRM has a forward "reader" style API
- SRM uses lots of structs, and you have to do an additional call to
lookup strings generally.
- SRM, as far as I have seen, doesn't have APIs to modify and write
out new assemblies.
- SRM only supports "portable" pdb files.
- SRM is not well documented yet. To discover usage, I read source
  code and/or unit tests.

From my benchmark above, it seems that SRM is 10x faster on
Windows/.NET framework and 5x faster on macOS/Mono.

So it makes sense for use to use SRM when reading assemblies (and we
don't need symbols), and continue with Mono.Cecil for the linker and
other things that modify assemblies.

There are a few places we can take advantage of SRM, but the simplest
with a reasonable impact was `ResolveAssemblies`:

    Before:
    320 ms  ResolveAssemblies                          1 calls
    After:
    112 ms  ResolveAssemblies                          1 calls

So a ~200ms savings on this MSBuild task, which runs on *every* build.
This was the Xamarin.Forms test project in this repo: a build with no
changes.

~~ Changes ~~

- Added a `MetadataResolver` type, as a way to cache `PEReader`
  instances. This is a comparable drop-in replacement for
  `DirectoryAssemblyResolver`.
- `MonoAndroidHelper.IsReferenceAssembly` now uses
  `System.Reflection.Metadata` instead of `Mono.Cecil`. This is used
  in a few other MSBuild tasks.
- A `MetadataExtensions` provides an extension method to simplify
  getting the full name of a custom attribute. We can add more here as
  needed.
- Had to adjust the filename reported for XA2002, should optionally
  call `Path.GetFileNameWithoutExtension` if the name ends with
  `.dll`.

The resulting code *should* be the same, except we are using SRM over
Mono.Cecil.

~~ Other changes ~~

[xabuild.exe] remove SRM reference

This appears to fix the build on macOS, we had this workaround from a
mono bump in the past. Since xabuild has its own version of
System.Reflection.Metadata that was already loaded, we weren't loading
the one we are using in XA's MSBuild tasks.

Things appear to work without the reference now.

~~ Downstream ~~

We will need to add the following assemblies to the installer:

- `System.Reflection.Metadata.dll`
- `System.Collections.Immutable.dll`
@jonathanpeppers
Copy link
Member Author

Windows build looks good, the single test failure is the same one on master right now.

{
string assemblyPath = assemblyName;
if (!assemblyPath.EndsWith (".dll", StringComparison.OrdinalIgnoreCase)) {
assemblyPath += ".dll";
Copy link
Member

Choose a reason for hiding this comment

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

Should this also support .exe suffixes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since e390702 we dropped support for referencing exe's at the MSBuild level. I didn't add support for it here, but if there is some scenario you think we'll hit, I can add it.

Offhand I can't think of a case where a .dll would reference a random .exe?

@jonpryor
Copy link
Member

Then I hit the blocker where SRM only supports portable PDB files for symbols

Is this really a blocker? Requiring Portable PDB files will only break two scenarios:

  1. Devs on macOS using mcs to compile, resulting in .dll.mdb files.
  2. Devs on Windows using "legacy" Debug symbols.

(1) should be nonexistent at this point.

(2) would be "harmed", but in return for no longer getting filename + line number information, their builds will (presumably) be significantly faster. This should be a desirable tradeoff.

That said, it would be interesting to know how many "currently used" projects aren't using Portable PDB files...

@jonathanpeppers
Copy link
Member Author

The ppdb-only thing was just a blocker for this code: https://github.com/xamarin/java.interop/blob/6ce89c77949847f67b788c657a97beb626802982/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/JavaCallableWrapperGenerator.cs#L206-L227

We could make it just "not have the information" if the pdb wasn't there.

@jonathanpeppers
Copy link
Member Author

Windows build still looks good, the only test failure is the SwitchBetweenDesignTimeBuild that is failing on master.

@jonpryor jonpryor merged commit c22475d into dotnet:master Jan 16, 2019
@jonathanpeppers jonathanpeppers deleted the resolveassemblies-srm branch May 20, 2019 19:59
@github-actions github-actions bot locked and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants