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] reduce task time #2093

Merged
merged 1 commit into from
Aug 26, 2018

Conversation

StephaneDelcroix
Copy link
Contributor

@StephaneDelcroix StephaneDelcroix commented Aug 22, 2018

This commit does 3 changes to ConvertResourceCase, in order to
drastically reduces execution times. On my sample, execution times are
less than half of the original times:

  • simplify the linq query used to get the xmls files (thx @jonathanpeppers)
  • compute reosurcedictionaries out of the loop
  • do not create a temp file as UpdateXmlResource already creates a temp
    file. Change the return value of UpdateXmlResource to indicate if the
    file has been changed or not.

@jonathanpeppers jonathanpeppers changed the title [Xamarin.Andorid.Build.Tasks] reduce task time [Xamarin.Android.Build.Tasks] reduce task time Aug 22, 2018
@jonathanpeppers jonathanpeppers force-pushed the convertResourceCasePerf branch from 0c2bca6 to 9527ffc Compare August 22, 2018 19:57
@jonathanpeppers
Copy link
Member

I think the removal of the temp files are breaking something:

  EXEC : 1) error : Xamarin.Android.Build.Tests.AndroidUpdateResourcesTest.Check9PatchFilesAreProcessed(True) [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/build-tools/scripts/RunTests.targets]
  MainActivity.cs(22,36,22,40): error CS0117: 'Resource.Layout' does not contain a definition for 'Main' [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/Check9PatchFilesAreProcessed_True/Application1/UnnamedProject.csproj] [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/build-tools/scripts/RunTests.targets]
  MainActivity.cs(26,54,26,62): error CS0117: 'Resource.Id' does not contain a definition for 'myButton' [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/Check9PatchFilesAreProcessed_True/Application1/UnnamedProject.csproj] [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/build-tools/scripts/RunTests.targets]
  MainActivity.cs(22,36,22,40): error CS0117: 'Resource.Layout' does not contain a definition for 'Main' [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/Check9PatchFilesAreProcessed_True/Application1/UnnamedProject.csproj] [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/build-tools/scripts/RunTests.targets]
  MainActivity.cs(26,54,26,62): error CS0117: 'Resource.Id' does not contain a definition for 'myButton' [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/TestDebug/temp/Check9PatchFilesAreProcessed_True/Application1/UnnamedProject.csproj] [/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/build-tools/scripts/RunTests.targets]
  /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/build-tools/scripts/RunTests.targets(38,5): error MSB3073: The command "mono packages/NUnit.ConsoleRunner.3.7.0/tools/nunit3-console.exe  /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/build-tools/scripts/../../bin/TestDebug/Xamarin.Android.Build.Tests.dll  --labels=All --result="TestResult-Xamarin.Android.Build.Tests.xml;format=nunit2" --output="bin/TestDebug/TestOutput-Xamarin.Android.Build.Tests.txt"" exited with code 1.

    128 Warning(s)
    6 Error(s)

Will look into it tomorrow.

@dellis1972
Copy link
Contributor

@StephaneDelcroix can we revert just the Temp file changes and leave the Linq and directory changes in? We can look at the temp file Changes in a different PR.

@pierceboggan
Copy link

@StephaneDelcroix Could you share some binlogs with the speedup we see with this change?

@StephaneDelcroix
Copy link
Contributor Author

StephaneDelcroix commented Aug 23, 2018

@dellis1972 sure I could cut that in parts, but the biggest gain in speed is from avoiding the double-temp file situation

@pierceboggan sure

@dellis1972
Copy link
Contributor

@StephaneDelcroix yes, but it looks like the temp file changes broke a few tests. I would like to get the linq changes in asap, then we can focus a new PR on those temp changes. Otherwise the PR will be held up until it is green. Alternatively, fix the problem that the test is highlighting, but I suspect that will take a bit more time.

@StephaneDelcroix StephaneDelcroix force-pushed the convertResourceCasePerf branch from 9527ffc to 518186d Compare August 24, 2018 12:36
@StephaneDelcroix
Copy link
Contributor Author

@dellis1972 removed the breaking parts, creating a new PR for it

@pierceboggan

using xabuild, on this mbp13, on a blank forms + android project (netstandard2)

Caveat emptor: times not averaged

full binlogs in your mailbox

62d639b (master) : /t:rebuild =>bl0.binlog

Time Elapsed 00:00:33.52
...
886 ms XamlCTask 2 calls
907 ms GenerateJavaStubs 1 calls
1377 ms Csc 2 calls
2803 ms Javac 1 calls
2896 ms MSBuild 6 calls
2916 ms ResolveLibraryProjectImports 1 calls
3878 ms ConvertResourcesCases 2 calls
5719 ms Aapt 3 calls
8316 ms CompileToDalvik 1 calls

62d639b (master) : /t:build (no changes) => bl1.binlog

Time Elapsed 00:00:09.35
...
939 ms GenerateJavaStubs 1 calls
1289 ms MSBuild 5 calls
1758 ms ConvertResourcesCases 2 calls
2279 ms Aapt

62d639b (master) : /t:build (change in the NS project) =>bl2.binlog

Time Elapsed 00:00:12.56
...
830 ms XamlCTask 2 calls
870 ms GenerateJavaStubs 1 calls
1307 ms Csc 2 calls
1787 ms ConvertResourcesCases 2 calls
2299 ms Aapt 2 calls
2379 ms MSBuild

linq fixes : /t:rebuild =>bl3.binlog

Time Elapsed 00:00:29.61
...
842 ms XamlCTask 2 calls
904 ms GenerateJavaStubs 1 calls
1395 ms Csc 2 calls
1961 ms ResolveLibraryProjectImports 1 calls
2517 ms Javac 1 calls
2763 ms MSBuild 6 calls
3441 ms ConvertResourcesCases 2 calls
4398 ms Aapt 3 calls
7978 ms CompileToDalvik 1 calls

linq fixes : /t:build (no changes) =>bl4.binlog

Time Elapsed 00:00:08.82
...
860 ms GenerateJavaStubs 1 calls
1252 ms MSBuild 5 calls
1555 ms ConvertResourcesCases 2 calls
2165 ms Aapt 2 calls

linq fixes : /t:build (change in NS project)=>bl5.binlog

Time Elapsed 00:00:11.89
...
789 ms XamlCTask 2 calls
859 ms GenerateJavaStubs 1 calls
1302 ms Csc 2 calls
1578 ms ConvertResourcesCases 2 calls
2158 ms Aapt 2 calls
2322 ms MSBuild 5 calls

linq + temp fixes (not in this PR anymore) : /t:rebuild =>bl6.binlog

Time Elapsed 00:00:30.00
...
882 ms XamlCTask 2 calls
919 ms GenerateJavaStubs 1 calls
1343 ms Csc 2 calls
1864 ms ResolveLibraryProjectImports 1 calls
2362 ms ConvertResourcesCases 2 calls
2685 ms MSBuild 6 calls
2726 ms Javac 1 calls
5309 ms Aapt 3 calls
8441 ms CompileToDalvik 1 calls

linq + temp fixes : /t:build (no changes) =>bl7.binlog

Time Elapsed 00:00:08.68
...
896 ms GenerateJavaStubs 1 calls
1165 ms ConvertResourcesCases 2 calls
1302 ms MSBuild 5 calls
2268 ms Aapt 2 calls

linq + temp fixes : /t:build (changes in NS) =>bl8.binlog

Time Elapsed 00:00:11.85
...
803 ms XamlCTask 2 calls
901 ms GenerateJavaStubs 1 calls
1183 ms ConvertResourcesCases 2 calls
1308 ms Csc 2 calls
2286 ms Aapt 2 calls
2441 ms MSBuild 5 calls

@dellis1972
Copy link
Contributor

build

1 similar comment
@dellis1972
Copy link
Contributor

build


var lastUpdate = DateTime.MinValue;
if (!string.IsNullOrEmpty (AndroidConversionFlagFile) && File.Exists (AndroidConversionFlagFile)) {
lastUpdate = File.GetLastWriteTimeUtc (AndroidConversionFlagFile);
}
Log.LogDebugMessage (" AndroidConversionFlagFile modified: {0}", lastUpdate);

var resourcedirectories = ResourceDirectories.Where (s => s != item).Select(s => s.ItemSpec).ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

@StephaneDelcroix I think too much got taken out.

Something should use this variable?

Copy link
Contributor Author

@StephaneDelcroix StephaneDelcroix Aug 24, 2018

Choose a reason for hiding this comment

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

forgot to push. fixed

- simplify linq queries in ConvertResourceCases
@StephaneDelcroix StephaneDelcroix force-pushed the convertResourceCasePerf branch from 518186d to 2128c04 Compare August 24, 2018 20:27
@dellis1972 dellis1972 merged commit 508c1c1 into dotnet:master Aug 26, 2018
jonpryor pushed a commit that referenced this pull request Sep 5, 2018
- simplify linq queries in ConvertResourceCases
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 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.

4 participants