-
Notifications
You must be signed in to change notification settings - Fork 535
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] timestamp fixes for incremental builds #1930
Conversation
Context: https://github.com/xamarin/Xamarin.Forms/blob/42c07d1ae5aa56eb574b7d169499f1a9af7ec080/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj While working on build performance, I noticed two timestamp issues that are preventing some targets in Xamarin.Android from building incrementally. I spotted both of these while timing builds of the `Xamarin.Forms.ControlGallery.Android` project. It is a good test subject, because it builds PCLs, and seven (or so) Xamarin.Android library projects. ~~ _LinkAssembliesNoShrink ~~ While timing builds, I noticed the following when `_LinkAssembliesNoShrink` runs: Target Name=_LinkAssembliesNoShrink Project=PagesGallery.Droid.csproj Building target "_LinkAssembliesNoShrink" partially, because some output files are out of date with respect to their input files. [ResolvedUserAssemblies: Input=C:\Users\myuser\Desktop\Git\Xamarin.Forms\PagesGallery\PagesGallery.Droid\bin\Debug\PagesGallery.Droid.dll, Output=obj\\Debug\android\assets\PagesGallery.Droid.dll] Input file is newer than output file. ... Skip linking unchanged file: C:\Users\myuser\Desktop\Git\Xamarin.Forms\PagesGallery\PagesGallery.Droid\bin\Debug\PagesGallery.Droid.dll It seems there is a case where MSBuild is thinking the target needs to run (due to timestamp values), but the `LinkAssemblies` MSBuild task does not copy the file. It actually doesn't need to do any work at all here, since this was a build with no changes. The fix here is to apply a timestamp for files that get skipped. This prevents the target from running again when it doesn't need to. I also removed a spot where a `copysrc` variable looks like it was being set an extra time--did not seem to be needed. ~~ _GenerateJavaStubs ~~ After fixing `_LinkAssembliesNoShrink`, I noticed another timestamp issue: Building target "_GenerateJavaStubs" completely. Input file "obj\\Debug\android\assets\PagesGallery.Droid.dll" is newer than output file "obj\\Debug\android\typemap.jm". Looking at the `GenerateJavaStubs` MSBuild task, it was never setting the timestamp on either `typemap.mj` or `typemap.jm`. Fixing these two issues, I was able to get a build with no-changes in this Xamarin.Forms project down from ~23 seconds to ~13 seconds. I also updated the `CheckTimestamps` test to validate these changes.
@dellis1972 one other issue I'm noticing (some was code I wrote): if (MonoAndroidHelper.CopyIfChanged (src, dst))
MonoAndroidHelper.SetLastAccessAndWriteTimeUtc (dst, DateTime.UtcNow, Log); Should probably instead be: MonoAndroidHelper.CopyIfChanged (src, dst);
MonoAndroidHelper.SetLastAccessAndWriteTimeUtc (dst, DateTime.UtcNow, Log); Does the second one seem correct? If we choose that a file doesn't need to be copied, we should still update the timestamp so that If so, there might be a few places I need to fix... |
@@ -306,7 +306,8 @@ void UpdateWhenChanged (string path, Action<Stream> generator) | |||
var np = path + ".new"; | |||
using (var o = File.OpenWrite (np)) | |||
generator (o); | |||
Files.CopyIfChanged (np, path); | |||
MonoAndroidHelper.CopyIfChanged (np, path); | |||
MonoAndroidHelper.SetLastAccessAndWriteTimeUtc (path, DateTime.UtcNow, Log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dellis1972 I was looking at this code. Do we really need this .new
file that we delete afterward?
Can we just write to path
, update the timestamp, and that's it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this allot. If we generate code we don't write directly to the file, we instead use a temp file and use 'CopyIfChanged' this ensures that we don't update files that we don't need to. That said in this case we are setting the date time.. so that kinda makes it redundant.
Might be worth testing to see what impact it has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think using the .new
file might be right.
Consider:
generator(o)
writes to the original file and fails with some error- We could have a 1/2 complete file with a newer timestamp, and the target wouldn't run again
@@ -306,7 +306,8 @@ void UpdateWhenChanged (string path, Action<Stream> generator) | |||
var np = path + ".new"; | |||
using (var o = File.OpenWrite (np)) | |||
generator (o); | |||
Files.CopyIfChanged (np, path); | |||
MonoAndroidHelper.CopyIfChanged (np, path); | |||
MonoAndroidHelper.SetLastAccessAndWriteTimeUtc (path, DateTime.UtcNow, Log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this allot. If we generate code we don't write directly to the file, we instead use a temp file and use 'CopyIfChanged' this ensures that we don't update files that we don't need to. That said in this case we are setting the date time.. so that kinda makes it redundant.
Might be worth testing to see what impact it has.
…es occur Context: dotnet#1930 Since 3d999d3, `Fast Deployment` has been rebuilding the APK when it shouldn't. Apparently the `typemap.mj` and `typemap.jm` files' timestamps are used when deciding to repack/deploy the APK or not. After looking at my `CheckTimestamps` test, I don't think it was quite right. It should modify `MainActivity`'s type name so that these files will be updated. Just modifying code won't invalidate these files. Then the `GenerateJavaStubs` task should only update the timestamps if the file was actually changed, which should fix `Fast Deployment`.
…1930) Context: https://github.com/xamarin/Xamarin.Forms/blob/42c07d1ae5aa56eb574b7d169499f1a9af7ec080/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj While working on build performance, I noticed two timestamp issues that are preventing some targets in Xamarin.Android from building incrementally. I spotted both of these while timing builds of the `Xamarin.Forms.ControlGallery.Android` project. It is a good test subject, because it builds PCLs, and seven (or so) Xamarin.Android library projects. ~~ _LinkAssembliesNoShrink ~~ While timing builds, I noticed the following when `_LinkAssembliesNoShrink` runs: Target Name=_LinkAssembliesNoShrink Project=PagesGallery.Droid.csproj Building target "_LinkAssembliesNoShrink" partially, because some output files are out of date with respect to their input files. [ResolvedUserAssemblies: Input=C:\Users\myuser\Desktop\Git\Xamarin.Forms\PagesGallery\PagesGallery.Droid\bin\Debug\PagesGallery.Droid.dll, Output=obj\\Debug\android\assets\PagesGallery.Droid.dll] Input file is newer than output file. ... Skip linking unchanged file: C:\Users\myuser\Desktop\Git\Xamarin.Forms\PagesGallery\PagesGallery.Droid\bin\Debug\PagesGallery.Droid.dll It seems there is a case where MSBuild is thinking the target needs to run (due to timestamp values), but the `LinkAssemblies` MSBuild task does not copy the file. It actually doesn't need to do any work at all here, since this was a build with no changes. The fix here is to apply a timestamp for files that get skipped. This prevents the target from running again when it doesn't need to. I also removed a spot where a `copysrc` variable looks like it was being set an extra time--did not seem to be needed. ~~ _GenerateJavaStubs ~~ After fixing `_LinkAssembliesNoShrink`, I noticed another timestamp issue: Building target "_GenerateJavaStubs" completely. Input file "obj\\Debug\android\assets\PagesGallery.Droid.dll" is newer than output file "obj\\Debug\android\typemap.jm". Looking at the `GenerateJavaStubs` MSBuild task, it was never setting the timestamp on either `typemap.mj` or `typemap.jm`. Fixing these two issues, I was able to get a build with no-changes in this Xamarin.Forms project down from ~23 seconds to ~13 seconds. I also updated the `CheckTimestamps` test to validate these changes.
Context: https://github.com/xamarin/Xamarin.Forms/blob/42c07d1ae5aa56eb574b7d169499f1a9af7ec080/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj
While working on build performance, I noticed two timestamp issues
that are preventing some targets in Xamarin.Android from building
incrementally. I spotted both of these while timing builds of the
Xamarin.Forms.ControlGallery.Android
project. It is a good testsubject, because it builds PCLs, and seven (or so) Xamarin.Android
library projects.
_LinkAssembliesNoShrink
While timing builds, I noticed the following when
_LinkAssembliesNoShrink
runs:It seems there is a case where MSBuild is thinking the target needs to
run (due to timestamp values), but the
LinkAssemblies
MSBuild taskdoes not copy the file. It actually doesn't need to do any work at all
here, since this was a build with no changes.
The fix here is to apply a timestamp for files that get skipped. This
prevents the target from running again when it doesn't need to.
I also removed a spot where a
copysrc
variable looks like it wasbeing set an extra time--did not seem to be needed.
_GenerateJavaStubs
After fixing
_LinkAssembliesNoShrink
, I noticed another timestampissue:
Looking at the
GenerateJavaStubs
MSBuild task, it was never settingthe timestamp on either
typemap.mj
ortypemap.jm
.Fixing these two issues, I was able to get a build with no-changes in
this Xamarin.Forms project down from ~23 seconds to ~13 seconds. I
also updated the
CheckTimestamps
test to validate these changes.