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

[xamlc] new <XFXamlCValidateOnly> MSBuild property for Debug builds #7407

Merged
merged 1 commit into from Sep 12, 2019

Conversation

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Sep 5, 2019

Description of Change

XamlC is currently enabled for most Xamarin.Forms projects in Debug
and Release configurations. It enables faster startup/runtime
performance, XAML-syntax checking at build time--both quiet useful!

However, there is a build-time cost to using XamlC: each assembly is
loaded via Mono.Cecil, IL generated, and saved back to disk as an
additional step after Rosyln has compiled the assembly.

The proposal would be to add a new experimental MSBuild property, that
can be enabled for Debug mode such as:

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
  <!-- ... -->
  <XFXamlCValidateOnly>True</XFXamlCValidateOnly>
</PropertyGroup>

This would do the following:

  • OptimizeIL is skipped.
  • Assemblies are analyzed, but no changes written to disk.
  • No symbols are loaded via Mono.Cecil, we don't need them if we
    aren't writing back to disk!

This will improve build performance, and we don't lose the build-time
error checking for XAML.

Down the road, we could consider moving the MSBuild property to be the
default for Debug builds or change the Xamarin templates. I think it
would be wise to require developers to opt-in to try this out.

Results

I tested this change by building the ControlGallery after changing one
line of XAML:

msbuild Xamarin.Forms.ControlGallery.Android\Xamarin.Forms.ControlGallery.Android.csproj /clp:performancesummary /p:XFXamlCValidateOnly=True

Before:
1979 ms  XamlCTask                                  1 calls
After:
 923 ms  XamlCTask                                  1 calls

I only tested Debug builds.

Right, so it's faster. But let's keep the entire developer loop in
mind, how much slower is startup?

Before:
09-05 14:37:32.274  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s890ms
09-05 14:38:30.178  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s848ms
09-05 14:38:40.300  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s848ms
After:
09-05 14:40:38.512  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s894ms
09-05 14:40:55.497  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s856ms
09-05 14:41:03.754  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s897ms

After three runs, it seems this app suffers 25-50ms slowdown to
startup, and gains 1 second of build time improvement. A good net-win!

Other apps that have significantly more XAML will have different
results. I suspect the build time improvement will be even better, but
the hit to startup could be worse. With this setting opt-in, we can
experiment and find out.

Issues Resolved

n/a

API Changes

None

Platforms Affected

  • Core/XAML (all platforms)

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

I added unit tests. We could do further testing with a project with lots of XAML to see the performance impact.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
@jonathanpeppers jonathanpeppers requested a review from StephaneDelcroix Sep 5, 2019
@samhouts samhouts added the a/Xaml </> label Sep 5, 2019
@samhouts samhouts added this to In Review in vCurrent (4.4.0) Sep 5, 2019
@StephaneDelcroix

This comment has been minimized.

Copy link
Member

StephaneDelcroix commented Sep 6, 2019

the problem with this approach is that it might hide some bugs happening only with XamlC on until the app is in Release mode.

that will make bugs very hard to find, very hard to diagnose, and very angry developers with app failing for unknown reasons in the field.

the saved second isn't, IMHO, not worth that pain

@jonathanpeppers

This comment has been minimized.

Copy link
Member Author

jonathanpeppers commented Sep 6, 2019

the problem with this approach is that it might hide some bugs happening only with XamlC on until the app is in Release mode.

That's kind of the same as a lot of other things: AOT, Linking, Proguard/R8, device vs emulator. All these things can break your app in Release mode, but you might not be using them in Debug mode (some you shouldn't).

We may want to keep this an optional setting, and devs can decide if it helps their build time.

@akamud

This comment has been minimized.

Copy link
Contributor

akamud commented Sep 6, 2019

+1 for the optional setting. We do our UITests and PR validation in release mode, so we can have an automated way of validating everything that only happens in release (Linker, AOT, etc.).

@jonathanpeppers

This comment has been minimized.

Copy link
Member Author

jonathanpeppers commented Sep 7, 2019

So I had someone mention on Twitter that XamlC in their app takes 64s by itself...

https://twitter.com/muhaym/status/1170213056619536384?s=21

So that is the kind of app that might take advantage of this. I wonder how many lines of XAML it is.

@StephaneDelcroix

This comment has been minimized.

Copy link
Member

StephaneDelcroix commented Sep 9, 2019

64s 🤦‍♂

wish I could have that project for profiling...

Copy link
Member

StephaneDelcroix left a comment

let's do this

Xamarin.Forms.Build.Tasks/XamlCTask.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Build.Tasks/XamlCTask.cs Outdated Show resolved Hide resolved
.nuspec/Xamarin.Forms.targets Outdated Show resolved Hide resolved
@samhouts samhouts moved this from In Review to In Progress in vCurrent (4.4.0) Sep 11, 2019
@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:xamlc-fastbuild branch from 54d9a9a to 14ed824 Sep 11, 2019
@jonathanpeppers jonathanpeppers changed the title [xamlc] new <XamlCReadOnly> MSBuild property for Debug builds [xamlc] new <XFXamlCValidateOnly> MSBuild property for Debug builds Sep 11, 2019
@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:xamlc-fastbuild branch from 6519955 to b8e886f Sep 11, 2019
@StephaneDelcroix

This comment has been minimized.

Copy link
Member

StephaneDelcroix commented Sep 12, 2019

@jonathanpeppers could you rebase and retarget on 4.3? thanks

@samhouts samhouts changed the base branch from master to 4.3.0 Sep 12, 2019
@samhouts samhouts changed the base branch from 4.3.0 to master Sep 12, 2019
@jonathanpeppers jonathanpeppers changed the base branch from master to 4.3.0 Sep 12, 2019
XamlC is currently enabled for most Xamarin.Forms projects in `Debug`
and `Release` configurations. It enables faster startup/runtime
performance, XAML-syntax checking at build time--both quiet useful!

However, there is a build-time cost to using XamlC: each assembly is
loaded via Mono.Cecil, IL generated, and saved back to disk as an
additional step after Rosyln has compiled the assembly.

The proposal would be to add a new experimental MSBuild property, that
can be enabled for `Debug` mode such as:

    <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
      <!-- ... -->
      <XFXamlCValidateOnly>True</XFXamlCValidateOnly>
    </PropertyGroup>

This would do the following:

* `OptimizeIL` is skipped.
* Assemblies are analyzed, but no changes written to disk.
* No symbols are loaded via Mono.Cecil, we don't need them if we
  aren't writing back to disk!

This will improve build performance, and we don't lose the build-time
error checking for XAML.

Down the road, we could consider moving the MSBuild property to be the
default for `Debug` builds or change the Xamarin templates. I think it
would be wise to require developers to opt-in to try this out.

~~ Results ~~

I tested this change by building the ControlGallery after changing one
line of XAML:

    msbuild Xamarin.Forms.ControlGallery.Android\Xamarin.Forms.ControlGallery.Android.csproj /clp:performancesummary /p:XFXamlCValidateOnly=True

    Before:
    1979 ms  XamlCTask                                  1 calls
    After:
     923 ms  XamlCTask                                  1 calls

I *only* tested `Debug` builds.

Right, so it's faster. But let's keep the entire developer loop in
mind, how much slower is startup?

    Before:
    09-05 14:37:32.274  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s890ms
    09-05 14:38:30.178  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s848ms
    09-05 14:38:40.300  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s848ms
    After:
    09-05 14:40:38.512  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s894ms
    09-05 14:40:55.497  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s856ms
    09-05 14:41:03.754  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s897ms

After three runs, it seems this app suffers 25-50ms slowdown to
startup, and gains 1 second of build time improvement. A good net-win!

Other apps that have significantly more XAML will have different
results. I suspect the build time improvement will be even better, but
the hit to startup could be worse. With this setting opt-in, we can
experiment and find out.
@jonathanpeppers jonathanpeppers force-pushed the jonathanpeppers:xamlc-fastbuild branch from b8e886f to b2b53fc Sep 12, 2019
@samhouts samhouts removed this from In Progress in vCurrent (4.4.0) Sep 12, 2019
@samhouts samhouts added this to In Progress in v4.3.0 Sep 12, 2019
@samhouts samhouts merged commit 7efca8e into xamarin:4.3.0 Sep 12, 2019
1 of 2 checks passed
1 of 2 checks passed
xamarin-forms-ci in progress
Details
license/cla All CLA requirements met.
Details
v4.3.0 automation moved this from In Progress to Done Sep 12, 2019
@jonathanpeppers jonathanpeppers deleted the jonathanpeppers:xamlc-fastbuild branch Sep 12, 2019
@samhouts samhouts added this to the 4.3.0 milestone Sep 18, 2019
felipebaltazar added a commit to felipebaltazar/Xamarin.Forms that referenced this pull request Oct 16, 2019
…amarin#7407)

XamlC is currently enabled for most Xamarin.Forms projects in `Debug`
and `Release` configurations. It enables faster startup/runtime
performance, XAML-syntax checking at build time--both quiet useful!

However, there is a build-time cost to using XamlC: each assembly is
loaded via Mono.Cecil, IL generated, and saved back to disk as an
additional step after Rosyln has compiled the assembly.

The proposal would be to add a new experimental MSBuild property, that
can be enabled for `Debug` mode such as:

    <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
      <!-- ... -->
      <XFXamlCValidateOnly>True</XFXamlCValidateOnly>
    </PropertyGroup>

This would do the following:

* `OptimizeIL` is skipped.
* Assemblies are analyzed, but no changes written to disk.
* No symbols are loaded via Mono.Cecil, we don't need them if we
  aren't writing back to disk!

This will improve build performance, and we don't lose the build-time
error checking for XAML.

Down the road, we could consider moving the MSBuild property to be the
default for `Debug` builds or change the Xamarin templates. I think it
would be wise to require developers to opt-in to try this out.

~~ Results ~~

I tested this change by building the ControlGallery after changing one
line of XAML:

    msbuild Xamarin.Forms.ControlGallery.Android\Xamarin.Forms.ControlGallery.Android.csproj /clp:performancesummary /p:XFXamlCValidateOnly=True

    Before:
    1979 ms  XamlCTask                                  1 calls
    After:
     923 ms  XamlCTask                                  1 calls

I *only* tested `Debug` builds.

Right, so it's faster. But let's keep the entire developer loop in
mind, how much slower is startup?

    Before:
    09-05 14:37:32.274  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s890ms
    09-05 14:38:30.178  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s848ms
    09-05 14:38:40.300  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s848ms
    After:
    09-05 14:40:38.512  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s894ms
    09-05 14:40:55.497  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s856ms
    09-05 14:41:03.754  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s897ms

After three runs, it seems this app suffers 25-50ms slowdown to
startup, and gains 1 second of build time improvement. A good net-win!

Other apps that have significantly more XAML will have different
results. I suspect the build time improvement will be even better, but
the hit to startup could be worse. With this setting opt-in, we can
experiment and find out.
felipebaltazar added a commit to felipebaltazar/Xamarin.Forms that referenced this pull request Oct 16, 2019
…amarin#7407)

XamlC is currently enabled for most Xamarin.Forms projects in `Debug`
and `Release` configurations. It enables faster startup/runtime
performance, XAML-syntax checking at build time--both quiet useful!

However, there is a build-time cost to using XamlC: each assembly is
loaded via Mono.Cecil, IL generated, and saved back to disk as an
additional step after Rosyln has compiled the assembly.

The proposal would be to add a new experimental MSBuild property, that
can be enabled for `Debug` mode such as:

    <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
      <!-- ... -->
      <XFXamlCValidateOnly>True</XFXamlCValidateOnly>
    </PropertyGroup>

This would do the following:

* `OptimizeIL` is skipped.
* Assemblies are analyzed, but no changes written to disk.
* No symbols are loaded via Mono.Cecil, we don't need them if we
  aren't writing back to disk!

This will improve build performance, and we don't lose the build-time
error checking for XAML.

Down the road, we could consider moving the MSBuild property to be the
default for `Debug` builds or change the Xamarin templates. I think it
would be wise to require developers to opt-in to try this out.

~~ Results ~~

I tested this change by building the ControlGallery after changing one
line of XAML:

    msbuild Xamarin.Forms.ControlGallery.Android\Xamarin.Forms.ControlGallery.Android.csproj /clp:performancesummary /p:XFXamlCValidateOnly=True

    Before:
    1979 ms  XamlCTask                                  1 calls
    After:
     923 ms  XamlCTask                                  1 calls

I *only* tested `Debug` builds.

Right, so it's faster. But let's keep the entire developer loop in
mind, how much slower is startup?

    Before:
    09-05 14:37:32.274  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s890ms
    09-05 14:38:30.178  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s848ms
    09-05 14:38:40.300  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s848ms
    After:
    09-05 14:40:38.512  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s894ms
    09-05 14:40:55.497  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s856ms
    09-05 14:41:03.754  1169  1192 I ActivityManager: Displayed AndroidControlGallery.AndroidControlGallery/md546303760447087909496d02dc7b17ae8.Activity1: +3s897ms

After three runs, it seems this app suffers 25-50ms slowdown to
startup, and gains 1 second of build time improvement. A good net-win!

Other apps that have significantly more XAML will have different
results. I suspect the build time improvement will be even better, but
the hit to startup could be worse. With this setting opt-in, we can
experiment and find out.
@muhaym

This comment has been minimized.

Copy link

muhaym commented Nov 8, 2019

@StephaneDelcroix It's just matter of asking :D The project is already handed over to @davidortinau long ago, but the code has changed a lot, we could use the project still for improving XF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v4.3.0
  
Done
5 participants
You can’t perform that action at this time.