Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[XamlC] no longer use any reflection-base ImportReference #1899

Merged
merged 3 commits into from
Feb 26, 2018

Conversation

StephaneDelcroix
Copy link
Member

@StephaneDelcroix StephaneDelcroix commented Feb 16, 2018

Description of Change

[XamlC] no longer use any reflection-base ImportReference

reflection-base ImportReference -- we were using MethodBase and Type --
are importing the types present in the reflection context, at the time
of compilation. As the compilation happens on netstandard2.0, and our
assembly can now be netstandard1.0 again, those imported types are
failing to be resolved.

this changes always import references based on the assembly, or the
assembly references.

it might, or might not, give us another speed bump.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@StephaneDelcroix StephaneDelcroix force-pushed the xamlc_noreflection branch 2 times, most recently from 03c9073 to ca121ef Compare February 16, 2018 14:26
@StephaneDelcroix StephaneDelcroix added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Feb 16, 2018
@StephaneDelcroix StephaneDelcroix removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Feb 21, 2018
reflection-base ImportReference -- we were using MethodBase and Type --
are importing the types present in the reflection context, at the time
of compilation. As the compilation happens on netstandard2.0, and our
assembly can now be netstandard1.0 again, those imported types are
failing to be resolved.

this changes always import references based on the assembly, or the
assembly references.

it might, or might not, give us another speed bump.
@StephaneDelcroix StephaneDelcroix changed the base branch from master to 3.0.0 February 22, 2018 08:06
@jassmith
Copy link

Will get you benchmark times tomorrow

@PureWeen
Copy link
Contributor

PureWeen commented Feb 23, 2018

For the latest PR
I have this version installed

<PackageReference Include="Xamarin.Forms" Version="2.6.0.210032" />

and with that I'm able to compile netstandard 1.4 with the following build times

1>      124 ms  ResolveAssemblyReference                   1 calls
1>      137 ms  JoinItems                                  8 calls
1>      186 ms  Copy                                       2 calls
1>      232 ms  Csc                                        1 calls
1>     9808 ms  XamlCTask                                  1 calls

Against netstandard 2.0 I have the following build times

1>      161 ms  ResolveAssemblyReference                   1 calls
1>      183 ms  Copy                                       2 calls
1>      203 ms  JoinItems                                  8 calls
1>      990 ms  Csc                                        1 calls
1>     1290 ms  XamlCTask                                  1 calls

//I hate you, netstandard
if (type.assemblyName == "mscorlib" && type.clrNamespace == "System.Reflection")
return module.GetTypeDefinition(("System.Reflection", type.clrNamespace, type.typeName));
throw new Exception($"Failed to get typedef for {type}");
Copy link
Member

Choose a reason for hiding this comment

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

👮 This LOOKS like this exception happens as part of this if clause because of the indentation.

@rmarinho rmarinho merged commit 463714b into 3.0.0 Feb 26, 2018
@samhouts samhouts added this to the 3.0.0 milestone May 5, 2018
@StephaneDelcroix StephaneDelcroix deleted the xamlc_noreflection branch May 8, 2018 07:31
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.

5 participants