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] Install should skip Build when inside the IDE #2626

Merged
merged 2 commits into from
Jan 25, 2019

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jan 17, 2019

Context: https://github.com/xamarin/xamarin-android/wiki/IDE-Performance-Results
Context: https://github.com/jonathanpeppers/HelloWorld

When doing some performance measurements inside of Visual Studio, I
noticed we seem to have significant overhead in a build with no
changes.

In a "Hello World" Xamarin.Forms app:

Preparation Time: 00:03.9
Launch Time:      00:02.5

Where Preparation Time is everything MSBuild, and Launch Time is
the time it takes to start the Android application and attach the
debugger.

Preparation Time is effectively:

msbuild Foo.Android.csproj /p:BuildingInsideVisualStudio=True /t:Build
msbuild Foo.Android.csproj /p:BuildingInsideVisualStudio=True /t:Install

One concern here is that Install depends on SignAndroidPackage,
which depends on Build. We are doing a lot of MSBuild work here
twice, since MSBuild needs to run through certain targets twice and
decide they can be skipped. This work is "not free" and mostly
involved MSBuild evaluating properties and time stamps on files.

What I found we could do here is skip Build on the Install target
when $(BuildingInsideVisualStudio) is True. Due to the dependency
chain, this also affects SignAndroidPackage.

The minimal list of targets for SignAndroidPackage that still work:

  • _CreatePropertiesCache
  • ResolveReferences
  • _CopyPackage
  • _Sign

Initial results from the IDE show:

Preparation Time: 00:02.06s

This is a ~2 second saving on the inner dev loop!

MSBuild Tests

Since our MSBuild tests set $(BuildingInsideVisualStudio), a lot of
our tests might break. We might have to add an additional call to
Build in each failing test.

The way I worked around this was to make the CreateApkBuilder method
run Build,SignAndroidPackage by default. Originally there were 248
test failures.

I also did some other cleanup:

  • Added a ProjectBuilder.RunTarget method, so tests can more easily
    run custom targets and not modify the Target property:
    builder.RunTarget ("Foo").
  • Added ProjectBuilder.DesignTimeBuild.
  • Builder now has a BuildingInsideVisualStudio property to toggle
    the value as a command-line global property. We were putting this in
    the csproj, which seems a bit different than what IDEs are
    actually doing...

Also added a BuildOutsideVisualStudio test to verify the
SignAndroidPackage target works by itself outside of IDEs.

@jonathanpeppers jonathanpeppers added the do-not-merge PR should not be merged. label Jan 17, 2019
@dellis1972
Copy link
Contributor

I like this idea. We need to ping @sgmunn to see what the impact on VSForMac would be.

Oh and see if we can fix the 248 failed tests :P

2 second saving is awesome!

@sgmunn
Copy link

sgmunn commented Jan 18, 2019

Not sure I completely understand this. but what I think I got is that because the IDE generally builds the project before install, then invokes the install target that we can skip build during install.

we are assuming therefore that inside of VS that the build has been done when we invoke the install target. I think that makes sense, one side-effect benefit is that the IDE option to execute again without building will, actually, not build again.

@dellis1972
Copy link
Contributor

@sgmunn your reading of this PR is correct. Do you think any work would be needed on the Mac side of things to allow this to work? or will it just work as is now?

@sgmunn
Copy link

sgmunn commented Jan 18, 2019

I don't think there would be any work, I am pretty sure that it would work out of the box.

@jonathanpeppers jonathanpeppers force-pushed the install-target-inside-vs branch from 04bae28 to 0f7d405 Compare January 18, 2019 22:23
…he IDE

Context: https://github.com/xamarin/xamarin-android/wiki/IDE-Performance-Results
Context: https://github.com/jonathanpeppers/HelloWorld

When doing some performance measurements *inside* of Visual Studio, I
noticed we seem to have significant overhead in a build with no
changes.

In a "Hello World" Xamarin.Forms app:

    Preparation Time: 00:03.9
    Launch Time:      00:02.5

Where `Preparation Time` is everything MSBuild, and `Launch Time` is
the time it takes to start the Android application and attach the
debugger.

`Preparation Time` is effectively:

    msbuild Foo.Android.csproj /p:BuildingInsideVisualStudio=True /t:Build
    msbuild Foo.Android.csproj /p:BuildingInsideVisualStudio=True /t:Install

One concern here is that `Install` depends on `SignAndroidPackage`,
which depends on `Build`. We are doing a lot of MSBuild work here
twice, since MSBuild needs to run through certain targets twice and
decide they can be skipped. This work is "not free" and mostly
involved MSBuild evaluating properties and time stamps on files.

What I found we could do here is skip `Build` on the `Install` target
when `$(BuildingInsideVisualStudio)` is `True`. Due to the dependency
chain, this also affects `SignAndroidPackage`.

The minimal list of targets for `SignAndroidPackage` that still work:

- `_CreatePropertiesCache`
- `ResolveReferences`
- `_CopyPackage`
- `_Sign`

Initial results from the IDE show:

    Preparation Time: 00:02.06s

This is a ~2 second saving on the inner dev loop!

~~ MSBuild Tests ~~

Since our MSBuild tests set `$(BuildingInsideVisualStudio)`, a lot of
our tests might break. We might have to add an additional call to
`Build` in each failing test.

The way I worked around this was to make the `CreateApkBuilder` method
run `Build,SignAndroidPackage` by default. Originally there were 248
test failures.

I also did some other cleanup:

- Added a `ProjectBuilder.RunTarget` method, so tests can more easily
  run custom targets and not modify the `Target` property:
  `builder.RunTarget ("Foo")`.
- Added `ProjectBuilder.DesignTimeBuild`.
- `Builder` now has a `BuildingInsideVisualStudio` property to toggle
  the value as a command-line global property. We were putting this in
  the `csproj`, which seems a bit different than what IDEs are
  actually doing...

Also added a `BuildOutsideVisualStudio` test to verify the
`SignAndroidPackage` target works by itself *outside* of IDEs.
@jonathanpeppers jonathanpeppers force-pushed the install-target-inside-vs branch from 0f7d405 to 6f3a6ee Compare January 22, 2019 17:12
@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Jan 22, 2019

I manually tested the Install target behavior change in VS for Mac, and things seemed to work fine.

I felt the devloop was snappier, but I wasn't sure if there is a way to time the difference inside VS for Mac?

@jonathanpeppers jonathanpeppers removed the do-not-merge PR should not be merged. label Jan 22, 2019
@sgmunn
Copy link

sgmunn commented Jan 22, 2019

there might be telemetry events you can use, cc @Therzok @mrward - just unsure of the semantics of what's being timed in the events

@mrward
Copy link

mrward commented Jan 22, 2019

VSMac has telemetry for the Build and Clean MSBuild targets - not sure if that is enough here since other targets are not measured.

@jonathanpeppers
Copy link
Member Author

@sgmunn @mrward VS Windows has a telemetry event that measures the time it takes from the user hitting play (F5), to the debugger attached. Keeps up with time spent in MSBuild vs app startup and debugger.

Maybe adding an overall telemetry event to measure the "devloop" like this would be good in VS for Mac? Probably helpful for iOS, too.

@mrward
Copy link

mrward commented Jan 22, 2019

There's a build and deploy telemetry event which @iainx added but I do not know much about it. There is also some debugger telemetry events but they do not include the build time separately.

Tests using their own `LocalBuilder` should default `BuildingInsideVisualStudio` to `false`.
@jonathanpeppers
Copy link
Member Author

Error is:

/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/build-tools/scripts/TestApks.targets(128,5): warning MSB3073: The command "kill -KILL 64651" exited with code 1. [/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/tests/RunApkTests.targets]
...
"/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/build-tools/scripts/RunTests.targets" (RunAllTests target) (1) ->
(RunAllTests target) -> 
  /Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/build-tools/scripts/RunTests.targets(158,5): error : Execution of target RunApkTests exited with code 1.

    11261 Warning(s)
    1 Error(s)

Same happened on this PR: #2634

Copy link
Contributor

@dellis1972 dellis1972 left a comment

Choose a reason for hiding this comment

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

Looks ok.

@jonpryor
Copy link
Member

@jonathanpeppers wrote:

Error is ...

The analysis is incorrect; the kill -KILL 64651 is a warning, not an error, and thus cannot be responsible for generating the error.

This appears to be an actual error:

TestApks.targets(217,5): error : Input file 'logcat-Release-Mono.Android_TestsMultiDex.txt' doesn't exist.

This is a reoccurring issue within <ProcessLogcatTiming/>, which needs to be addressed at some point...

There's also this, which is a bit more concerning:

Xamarin.Android.Common.targets(2890,3): error : One or more errors occurred. (ApplicationName='/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/Darwin/cross-arm', ...
...
error XA3001: System.AggregateException: One or more errors occurred. ...

Looks like cross-arm (AOT) is erring out, but the normal log doesn't have many details about the actual AOT command and the corresponding errors. Thus, we turn to xa-build-status-v9.1.199.110_Darwin-x86_64_pr_9af43d4d-Release.zip for MOAR INPUT. The build log in xa-build-status-v9.1.199.110_Darwin-x86_64_pr_9af43d4d-Release/bin/BuildRelease/msbuild-20190122T175747-Mono.Android-Tests-AOT.binlog contains the AOT commands, e.g.:

  [AOT] MONO_PATH="/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/src/Mono.Android/Test/obj/Release/android/assets/shrunk" MONO_ENV_OPTIONS="" /Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/Darwin/cross-arm --aot=outfile=/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/src/Mono.Android/Test/obj/Release/aot/armeabi-v7a/libaot-Java.Interop.dll.so,asmwriter,mtriple=armv7-linux-gnueabi,tool-prefix=/Users/builder/android-toolchain/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/darwin-x86_64/bin/arm-linux-androideabi-,ld-flags=,llvm-path=/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/Darwin,temp-path=/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/src/Mono.Android/Test/obj/Release/aot/armeabi-v7a/Java.Interop.dll /Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/src/Mono.Android/Test/obj/Release/android/assets/shrunk/Java.Interop.dll

Unfortunately, I don't see an error message from cross-arm in the output; it just failed silently. :-(

(Now that I think of it, maybe <ProcessLogcatTiming/> is failing because cross-arm is failing... Not sure why I haven't seen that XA3001 error before...)

I don't see why this PR would break cross-arm. I'm re-trying the PR build.

@jonpryor jonpryor merged commit 76fb90e into dotnet:master Jan 25, 2019
@jonathanpeppers jonathanpeppers deleted the install-target-inside-vs branch January 29, 2019 22:37
@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.

5 participants