[Xamarin.Android.Build.Tasks] fix incremental builds for Xamarin.Forms projects #2088
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@dellis1972 when you're back (after I got this green), I want to check I'm doing the right thing with the two flag files passed to the two times It seemed like they were always "fighting" each other and updating the same file in a Xamarin.Forms app: in a build with no changes. |
jonathanpeppers
commented
Aug 22, 2018
| AcwMapFile="$(_AcwMapFile)" | ||
| AndroidConversionFlagFile="$(IntermediateOutputPath)_javastubs.stamp" | ||
| /> | ||
| <Touch Files="$(IntermediateOutputPath)_javastubs.stamp;$(_AndroidResgenFlagFile)" AlwaysCreate="True" /> |
There was a problem hiding this comment.
This is the part I was worried about, will touching $(_AndroidResgenFlagFile) have any ill effects?
This is what made this PR green, and prevented the two ConvertResourcesCases from fighting each other.
|
@jonathanpeppers we need to be careful around this area.
So if GenerateJavaStubs runs we currently need to then process ALL the resources again so fix up those views. That needs some thinking since I would love to figure out a way of not having to process ALL the files again.. just those which contains stuff that looks like custom views. To be honest I was gonna put in a Touch for the |
dellis1972
reviewed
Aug 23, 2018
| ManifestPlaceholders="$(AndroidManifestPlaceholders)" | ||
| OutputDirectory="$(IntermediateOutputPath)android" | ||
| MergedAndroidManifestOutput="$(IntermediateOutputPath)android\AndroidManifest.xml" | ||
| ManifestTemplate="$(_AndroidManifestAbs)" |
There was a problem hiding this comment.
@jonpryor is not gonna like all the space changes here. The rule is we need to be consistent with the existing code in that part of the file. This is until we can split this file up into smaller chunks and reformat :)
|
Noob question: So after fixing this issue, the situation described above now takes ~0s instead of xx seconds? Build |
|
@pierceboggan on @StephaneDelcroix's machine: NOTE: this was a Debug/master build of Xamarin.Android These were running even if your build had no changes. These won't run anymore if you have no changes. It looked to me this file that is in the Xamarin.Forms template causes it: <android.support.design.widget.TabLayout
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:app="http://schemas.android.com/apk/res-auto"
android:id="@+id/sliding_tabs"
android:background="?attr/colorPrimary\"
android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar"
app:tabIndicatorColor="@android:color/white"
app:tabGravity="fill" app:tabMode="fixed" />It wasn't happening in a "Hello World" app, or the existing tests in Xamarin.Android. |
jonathanpeppers
force-pushed
the
incremental-timestamps
branch
from
ae72d08
to
1f862e8
Compare
|
Looking into these new test failures on Mac... It was green, then all I did was rebase + undo formatting changes? |
|
@jonpryor this is an example of the test failures grendel was talking about: |
|
@jonathanpeppers: wrt your previous comment, mono apparently doesn't reliably like (or dislike) whatever our unit tests are doing: Sometimes they cause mono to crash. I'm not sure if these are mono bugs or Java.Interop bugs; they require investigation. |
…s projects @StephaneDelcroix noticed a build performance problem in Xamarin.Forms projects: - Build - Build again with no changes - Slow targets such as `_UpdateAndroidResgen` and `_GenerateJavaStubs` run again! The cause appeared to be this: Building target "_UpdateAndroidResgen" completely. Input file "obj/Debug/res/layout/tabbar.xml" is newer than output file "obj/Debug/R.cs.flag". I modified the `CheckTimestamps` test to replicate this issue: - Added Xamarin.Forms NuGet packages - Added `Tabbar.axml` from the Xamarin.Forms template This surfaced several problems with timestamps and Xamarin.Android building incrementally... ~~ Problem 1 ~~ In the `_GenerateJavaStubs` target, the `ConvertResourcesCases` MSBuild task is not supplied a `AndroidConversionFlagFile`. It uses this file to compare timestamps and decide if XML files need fixed up or not. Supplying `$(_AndroidResgenFlagFile)` seemed to be the way to go here, since `_UpdateAndroidResgen` runs right before this task. It also solves the original problem from the `Tabbar.axml` file. I also realized we should be using a "stamp" file for the `_GenerateJavaStubs` MSBuild target, in general. This gives us several benefits: - `ConvertResourcesCases` has a proper `AndroidConversionFlagFile` it can use for checking timestamps - The `Outputs` of the target are drastically simplified, this should help peformance in MSBuild evaluating if the target should run - It is likely more precise/correct. So I added a new file in `$(IntermediateOutputPath)`, `obj\Debug\_javastubs.stamp` we use to determine if `_GenerateJavaStubs` should run. I also tried to do things "the right way", such as: - Adding the stamp file to the `FileWrites` item group - Made sure the file is deleted during a `Clean` After these changes, two instances of `ConvertResourcesCases` were *still* fighting each other. They would always run and update the timestamps on one file: causing the other one to run. `ConvertResourcesCases` runs twice: - In the `_UpdateAndroidResgen` target - In the `_GenerateJavaStubs` target But these are both working with the same files, so in the second case, we need to make sure `_GenerateJavaStubs` updates the timestamp on the *first* flag file so the target doesn't run every time. ~~ Problem 2 ~~ Timestamps of Xamarin.Forms assemblies in `obj\Debug\linksrc` were out of date! I updated the `_CopyIntermediateAssemblies` target to update the timestamps of all files within this directory. It did not appear to be doing it correctly, and was only running `<Touch />` on a subset of assemblies. ~~ Problem 3 ~~ After fixing No. 2, the next problem I noticed were `*.pdb` and `*.mdb` files in `obj\Debug\linksrc` were out of date! The problem here was a call to `<Touch />`: <Touch Files="@(_DebugFilesCopiedToLinkerSrc)" /> The `_DebugFilesCopiedToLinkerSrc` did not appear to contain any items! I updated the `_CopyPdbFiles` and `_CopyMdbFiles` targets to use the correct item groups. ~~ Problem 4 ~~ Lastly, after fixing the previous three issues, the timestamps of output assemblies in `obj\Release\android\assets` were not correct for `Release` builds. I had made a change for this in the past, which was making `Debug` builds work correctly: dotnet#2028 At the time I wrote dotnet#2028, since the test wasn't using Xamarin.Forms, I did not see the problem ocurring in `Release` mode. It *does* occur if you are using NuGet packages. I replicated what I changed in the `_LinkAssembliesNoShrink` target, and also renamed the item groups to be unique: - `_LinkAssembliesNoShrinkFiles` - `_LinkAssembliesShrinkFiles` ~~ Other changes ~~ The `CheckTimestamps` test also was verifying timestamps in `$(OutputPath)` such as `bin\Debug`. It appears the core MSBuild targets are leaving timestamps as-is in the `CopyFilesToOutputDirectory` and `_CopyFilesMarkedCopyLocal` targets. Assemblies from Xamarin.Forms NuGet are out of date here, but I don't think we should do anything, since the core MSBuild targets are doing this. For now, I removed the checks for `$(OutputPath)` in this test, and cleaned up the test a bit.
This fixes the failing test on MacOS. It appears I'm seeing diffent behavior running on Mono, and I'm having to explicitly set the timestamps on files being extracted from a zip. I also couldn't use `MonoAndroidHelper`, since it's method that does this requires the MSBuild logger.
jonathanpeppers
force-pushed
the
incremental-timestamps
branch
from
c6090ca
to
1ba55d7
Compare
|
Rebasing and hoping we can get a green build here. |
dellis1972
approved these changes
Aug 31, 2018
jonathanpeppers
added a commit
to jonathanpeppers/xamarin-android
that referenced
this pull request
Aug 31, 2018
…Changed Context: dotnet#2088 970da9e was a good step towards "correctness" in building incrementally in the following scenario: - File | New Xamarin.Forms project | NetStandard library - Build - Change XAML - Build In this scenario, there is now a new target rising to the surface we can improve: 276 ms _CopyIntermediateAssemblies 1 calls Looking at the target, it seems we could use the `CopyIfChanged` task here more effectively. This task will automaticaly set the timestamps of files that have been copied, and so we don't need any subsequent `<ItemGroup />` or `<Touch />` elements. It was also touching *all* files instead of just the ones that were changed. After this change: 33 ms _CopyIntermediateAssemblies 1 calls The overall build went from 7.058s to 6.652s, so there must be some other targets that benefit from the timestamps not changing on *all* of these files.
dellis1972
pushed a commit
that referenced
this pull request
Sep 3, 2018
…Changed (#2128) Context: #2088 970da9e was a good step towards "correctness" in building incrementally in the following scenario: - File | New Xamarin.Forms project | NetStandard library - Build - Change XAML - Build In this scenario, there is now a new target rising to the surface we can improve: 276 ms _CopyIntermediateAssemblies 1 calls Looking at the target, it seems we could use the `CopyIfChanged` task here more effectively. This task will automaticaly set the timestamps of files that have been copied, and so we don't need any subsequent `<ItemGroup />` or `<Touch />` elements. It was also touching *all* files instead of just the ones that were changed. After this change: 33 ms _CopyIntermediateAssemblies 1 calls The overall build went from 7.058s to 6.652s, so there must be some other targets that benefit from the timestamps not changing on *all* of these files.
|
Looks like this change affects the runtime performance, https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/plot/Tests%20times/ - check the XForms sample times. Do you know what might have caused that? |
jonpryor
pushed a commit
that referenced
this pull request
Sep 5, 2018
…s projects (#2088) @StephaneDelcroix noticed a build performance problem when building Xamarin.Forms projects: - Build - Build again with no changes - Slow targets such as `_UpdateAndroidResgen` and `_GenerateJavaStubs` run again! The cause appeared to be this: Building target "_UpdateAndroidResgen" completely. Input file "obj/Debug/res/layout/tabbar.xml" is newer than output file "obj/Debug/R.cs.flag". I modified the `CheckTimestamps()` test to replicate this issue: - Added Xamarin.Forms NuGet packages - Added `Tabbar.axml` from the Xamarin.Forms template This surfaced several problems with timestamps and Xamarin.Android building incrementally. ~~ Problem 1 ~~ In the `_GenerateJavaStubs` target, the `<ConvertResourcesCases/>` MSBuild task is not supplied an `AndroidConversionFlagFile`. It uses this file to compare timestamps and decide if XML files need to be fixed up or not. Supplying `$(_AndroidResgenFlagFile)` seemed to be the way to go here, since `_UpdateAndroidResgen` runs right before this task. It also solves the original problem from the `Tabbar.axml` file. I also realized we should be using a "stamp" file for the `_GenerateJavaStubs` MSBuild target, in general. This gives us several benefits: - `<ConvertResourcesCases/>` has a proper `AndroidConversionFlagFile` it can use for checking timestamps - The `Outputs` of the target are drastically simplified, which may help performance in MSBuild evaluating if the target should run - It is likely more precise/correct. So I added a new file in `$(IntermediateOutputPath)`, `obj\Debug\_javastubs.stamp` we use to determine if `_GenerateJavaStubs` should run. I also tried to do things "the right way", such as: - Adding the stamp file to the `@(FileWrites)` item group - Made sure the file is deleted during a `Clean` After these changes, two instances of `<ConvertResourcesCases/>` were *still* fighting each other. They would always run and update the timestamps on one file, causing the other one to run. `ConvertResourcesCases` runs twice: - In the `_UpdateAndroidResgen` target - In the `_GenerateJavaStubs` target But these are both working with the same files, so in the second case, we need to make sure `_GenerateJavaStubs` updates the timestamp on the *first* flag file so the target doesn't run every time. ~~ Problem 2 ~~ Timestamps of Xamarin.Forms assemblies in `obj\Debug\linksrc` were out of date! I updated the `_CopyIntermediateAssemblies` target to update the timestamps of all files within this directory. It did not appear to be doing it correctly, and was only running `<Touch/>` on a subset of assemblies. ~~ Problem 3 ~~ After fixing Problem 2, the next problem I noticed were `*.pdb` and `*.mdb` files in `obj\Debug\linksrc` were out of date! The problem here was a call to `<Touch />`: <Touch Files="@(_DebugFilesCopiedToLinkerSrc)" /> The `@(_DebugFilesCopiedToLinkerSrc)` did not appear to contain any items! I updated the `_CopyPdbFiles` and `_CopyMdbFiles` targets to use the correct item groups. ~~ Problem 4 ~~ Lastly, after fixing the previous three problems, the timestamps of output assemblies in `obj\Release\android\assets` were not correct for `Release` builds. I had made a change for this in the past, which was making `Debug` builds work correctly: #2028 At the time I wrote #2028, since the test wasn't using Xamarin.Forms, I did not see the problem occurring in `Release` mode. It *does* occur if you are using NuGet packages. I replicated what I changed in the `_LinkAssembliesNoShrink` target, and also renamed the item groups to be unique: - `_LinkAssembliesNoShrinkFiles` - `_LinkAssembliesShrinkFiles` ~~ Other changes ~~ The `CheckTimestamps()` test also was verifying timestamps in `$(OutputPath)` such as `bin\Debug`. It appears the core MSBuild targets are leaving timestamps as-is in the `CopyFilesToOutputDirectory` and `_CopyFilesMarkedCopyLocal` targets. Assemblies from Xamarin.Forms NuGet are out of date here, but I don't think we should do anything, since the core MSBuild targets are doing this. For now, I removed the checks for `$(OutputPath)` in this test, and cleaned up the test a bit. Update `Files.ExtractAll()` so that the timestamps of extracted files are set to "now", in order to fix some failing unit tests on macOS.
|
@radekdoulik, my thoughts on the graph for startup time:
#1150 is this commit. #1149 has a few commits, but most notably updates our emulator here from API 21 to API 28. This commit also has a slower startup, we seem to have a lot of variance now... I'm wondering if there is a slower code path in Xamarin.Forms for newer API levels? I've been testing 15.9 today, and seems like XamlC is still working fine? I'm not sure what could have any impact to startup time here.... |
|
Yes, might be related to the emulator updates. I also plan to fix the Bcl test measurements to see if it might be fluctuating as well? (it is longer running test exercising more of our code) Which might give us better indication whether it is emulator related or Xamarin.Forms related. |
jonathanpeppers
added a commit
to jonathanpeppers/xamarin-android
that referenced
this pull request
Sep 21, 2018
Context: dotnet#2088 Context: dotnet#2129 Context: dotnet#2199 In 4deec52, I fixed the #deletebinobj problem we discovered. However... I introduced a regression to incremental builds, I noticed that the `_CompileJava` target is now running on a second build with no changes. Third build? oddly it gets skipped... It seems to be due to our use of flag files: 1. `_UpdateAndroidResgen` updates `R.cs.flag` and uses the file as an output 2. `_GenerateJavaDesignerForComponent` now uses `R.cs.flag` as an input 3. `_GenerateJavaStubs` *also* updates the timestamp on `R.cs.flag`. This was added in 970da9e, as a workaround for our two instances of `ConvertResourcesCases`. 4. `_GenerateJavaDesignerForComponent` will now run again on the next build. Since 1886e6f eliminated the second call to `ConvertResourcesCases`, we don't need to update `R.cs.flag` in no. 3 any longer. Removing the call to `<Touch />` `R.cs.flag` in `_GenerateJavaStubs` fixed the issue, and I added some assertions in relevant tests to check that the `_CompileJava` and `_GenerateJavaDesignerForComponent` targets aren't running on incremental builds.
dellis1972
pushed a commit
that referenced
this pull request
Sep 21, 2018
Context: #2088 Context: #2129 Context: #2199 In 4deec52, I fixed the #deletebinobj problem we discovered. However... I introduced a regression to incremental builds, I noticed that the `_CompileJava` target is now running on a second build with no changes. Third build? oddly it gets skipped... It seems to be due to our use of flag files: 1. `_UpdateAndroidResgen` updates `R.cs.flag` and uses the file as an output 2. `_GenerateJavaDesignerForComponent` now uses `R.cs.flag` as an input 3. `_GenerateJavaStubs` *also* updates the timestamp on `R.cs.flag`. This was added in 970da9e, as a workaround for our two instances of `ConvertResourcesCases`. 4. `_GenerateJavaDesignerForComponent` will now run again on the next build. Since 1886e6f eliminated the second call to `ConvertResourcesCases`, we don't need to update `R.cs.flag` in no. 3 any longer. Removing the call to `<Touch />` `R.cs.flag` in `_GenerateJavaStubs` fixed the issue, and I added some assertions in relevant tests to check that the `_CompileJava` and `_GenerateJavaDesignerForComponent` targets aren't running on incremental builds.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Xamarin.Android.Build.Tasks] fix incremental builds for Xamarin.Forms projects
@StephaneDelcroix noticed a build performance problem in Xamarin.Forms
projects:
_UpdateAndroidResgenand_GenerateJavaStubsrun again!
The cause appeared to be this:
I modified the
CheckTimestampstest to replicate this issue:Tabbar.axmlfrom the Xamarin.Forms templateThis surfaced several problems with timestamps and Xamarin.Android
building incrementally...
Problem 1
In the
_GenerateJavaStubstarget, theConvertResourcesCasesMSBuild task is not supplied a
AndroidConversionFlagFile. It usesthis file to compare timestamps and decide if XML files need fixed up
or not.
Supplying
$(_AndroidResgenFlagFile)seemed to be the way to go here,since
_UpdateAndroidResgenruns right before this task. It alsosolves the original problem from the
Tabbar.axmlfile.I also realized we should be using a "stamp" file for the
_GenerateJavaStubsMSBuild target, in general.This gives us several benefits:
ConvertResourcesCaseshas a properAndroidConversionFlagFileitcan use for checking timestamps
Outputsof the target are drastically simplified, this shouldhelp peformance in MSBuild evaluating if the target should run
So I added a new file in
$(IntermediateOutputPath),obj\Debug\_javastubs.stampwe use to determine if_GenerateJavaStubsshould run.I also tried to do things "the right way", such as:
FileWritesitem groupCleanAfter these changes, two instances of
ConvertResourcesCaseswerestill fighting each other. They would always run and update the
timestamps on one file: causing the other one to run.
ConvertResourcesCasesruns twice:_UpdateAndroidResgentarget_GenerateJavaStubstargetBut these are both working with the same files, so in the second case,
we need to make sure
_GenerateJavaStubsupdates the timestamp on thefirst flag file so the target doesn't run every time.
Problem 2
Timestamps of Xamarin.Forms assemblies in
obj\Debug\linksrcwere outof date!
I updated the
_CopyIntermediateAssembliestarget to update thetimestamps of all files within this directory. It did not appear to be
doing it correctly, and was only running
<Touch />on a subset ofassemblies.
Problem 3
After fixing No. 2, the next problem I noticed were
*.pdband*.mdbfiles inobj\Debug\linksrcwere out of date!The problem here was a call to
<Touch />:The
_DebugFilesCopiedToLinkerSrcdid not appear to contain anyitems!
I updated the
_CopyPdbFilesand_CopyMdbFilestargets to use thecorrect item groups.
Problem 4
Lastly, after fixing the previous three issues, the timestamps of
output assemblies in
obj\Release\android\assetswere not correct forReleasebuilds.I had made a change for this in the past, which was making
Debugbuilds work correctly:
#2028
At the time I wrote #2028, since the test wasn't using Xamarin.Forms,
I did not see the problem ocurring in
Releasemode. It does occurif you are using NuGet packages.
I replicated what I changed in the
_LinkAssembliesNoShrinktarget,and also renamed the item groups to be unique:
_LinkAssembliesNoShrinkFiles_LinkAssembliesShrinkFilesOther changes
The
CheckTimestampstest also was verifying timestamps in$(OutputPath)such asbin\Debug.It appears the core MSBuild targets are leaving timestamps as-is in
the
CopyFilesToOutputDirectoryand_CopyFilesMarkedCopyLocaltargets. Assemblies from Xamarin.Forms NuGet are out of date here, but
I don't think we should do anything, since the core MSBuild targets
are doing this.
For now, I removed the checks for
$(OutputPath)in this test, andcleaned up the test a bit.