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.

Sign up for GitHub

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

Jump to bottom

[XABT] Scan for JCWs for each ABI in parallel. #9215

Merged
merged 2 commits into from
Aug 20, 2024
Merged

[XABT] Scan for JCWs for each ABI in parallel. #9215

merged 2 commits into from
Aug 20, 2024

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Aug 16, 2024 •
edited
Loading

In order to ensure that all of our per-arch assemblies contain the same JCW types, we scan them all. We can parallelize this operation per-ABI to save time. It is ok to parallelize Cecil here, as each "set" contains completely different assemblies coming from completely different assembly resolvers.

Fresh dotnet build of Android template (defaults to 4 architectures), total GenerateJavaSourcesAndMaybeClassifyMarshalMethods takes:
main 11835 ms
PR 4548 ms

@jpobst jpobst force-pushed the parallel-jcw branch 3 times, most recently from 5bca7ef to c1a7daa Compare August 16, 2024 20:13
jonathanpeppers
jonathanpeppers reviewed Aug 16, 2024
View reviewed changes
src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
var generateSucceeded = true;

// Generate Java sources in parallel
Parallel.ForEach (allAssembliesPerArch, (kvp) => {
Copy link
Member

Choose a reason for hiding this comment

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

Does this use Mono.Cecil in parallel? We found problems using Mono.Cecil on different threads in:

Random, weird things would happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we are fine here. While we are using the Mono.Cecil library in parallel, the instances running on not using the same assemblies or AssemblyResolver.

That is, one thread is generating all JCWs for arm64 from the arm64 assemblies:

  • obj\Debug\net9.0-android\android\assets\arm64-v8a\Mono.Android.dll
  • obj\Debug\net9.0-android\android\assets\arm64-v8a\Mono.Android.Runtime.dll
  • obj\Debug\net9.0-android\android\assets\arm64-v8a\Mono.Android.Export.dll

while another is generating all JCWs from the x86 assemblies:

  • obj\Debug\net9.0-android\android\assets\x86_64\Mono.Android.dll
  • obj\Debug\net9.0-android\android\assets\x86_64\Mono.Android.Runtime.dll
  • obj\Debug\net9.0-android\android\assets\x86_64\Mono.Android.Export.dll

No AssemblyResolver, AssemblyDefinition, or TypeDefinition instances are being accessed from more than one thread at a time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, we should be fine here. I designed the per-RID support so that, eventually, it can be parallelized. All the caches etc are per-arch (and if anything isn't, then it's either read-only or a bug).

@jpobst jpobst marked this pull request as ready for review August 16, 2024 22:40
@jpobst jpobst requested a review from dellis1972 as a code owner August 16, 2024 22:40
@jpobst jpobst requested review from jonpryor and grendello August 16, 2024 22:40
grendello
grendello reviewed Aug 19, 2024
View reviewed changes
src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs Show resolved Hide resolved
src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs Show resolved Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/JCWGenerator.cs
@@ -220,7 +221,7 @@ static string GetMonoInitSource (string androidSdkPlatform)
return builder.ToString ();
}

public static void EnsureAllArchitecturesAreIdentical (TaskLoggingHelper logger, Dictionary<AndroidTargetArch, NativeCodeGenState> javaStubStates)
public static void EnsureAllArchitecturesAreIdentical (TaskLoggingHelper logger, ConcurrentDictionary<AndroidTargetArch, NativeCodeGenState> javaStubStates)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test performance with a lock instead of using ConcurrentDictionary? A monitor might perform better than any concurrent collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not. We only store a maximum of 4 items in the Dictionary, so I'm not sure any operations on it will be measurable.

@jonpryor
Copy link
Member

We have a unit test failure; not sure if it's related to this PR:

Mono.Android.NET_Tests, Xamarin.Android.NetTests.AndroidMessageHandlerNegotiateAuthenticationTests.RequestWithoutCredentialsFails / Release:

System.NullReferenceException : Object reference not set to an instance of an object

   at Xamarin.Android.NetTests.AndroidMessageHandlerNegotiateAuthenticationTests.FakeNtlmServer.Loop()
   at NUnit.Framework.Internal.AsyncInvocationRegion.AsyncTaskInvocationRegion.WaitForPendingOperationsToComplete(Object )
   at NUnit.Framework.Internal.Commands.TestMethodCommand.RunAsyncTestMethod(TestExecutionContext )

I've restarted the tests.

@jonpryor jonpryor merged commit 79aab5c into main Aug 20, 2024
58 checks passed
@jonpryor jonpryor deleted the parallel-jcw branch August 20, 2024 19:26
grendello added a commit that referenced this pull request Aug 27, 2024
@grendello
Merge branch 'main' into dev/grendel/perfetto-integration
9d10266 
* main: (47 commits)
  Bump to dotnet/sdk@5642787dac 9.0.100-rc.2.24426.2 (#9247)
  LEGO: Merge pull request 9246
  Bump to 34.0.137 of the .NET 8 Android workload (#9243)
  Bump external/Java.Interop from `d30d554` to `51b784a` (#9241)
  Bump dotnet/android-tools@6575743 (#9235)
  Bump to mono/debugger-libs@d5664344 (#9238)
  [ci] Improve push_signed_nugets job condition (#9240)
  Bump to dotnet/android-tools@657574378a6 and xamarin/monodroid@8bd4bae7 (#9216)
  Bump to dotnet/java-interop@d30d554 (#9234)
  Localized file check-in by OneLocBuild Task (#9236)
  Bump to dotnet/sdk@e2b7b9d2b4 9.0.100-rc.2.24420.1 (#9228)
  $(AndroidPackVersionSuffix)=rc.2; net9 is 35.0.0-rc.2 (#9233)
  [Xamarin.Android.Build.Tasks] Scan for JCWs for each ABI in parallel (#9215)
  [Xamarin.Android.Build.Tasks] %(JavaArtifact) is a list (#9112)
  [native/monodroid] Fix error demangling satellite assembly names (#9166)
  [build] Update package metadata (#9230)
  [Xamarin.Android.Build.Tasks] Remove ILRepack (#9226)
  [Xamarin.Android.Build.Tasks] Fix an issue with incremental builds (#9183)
  [Xamarin.Android.Build.Tasks] remove `$XAMARIN_BUILD_ID` (#9223)
  [Mono.Android] Use .NET version of mdoc (#9225)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers approved these changes

@dellis1972 dellis1972 dellis1972 approved these changes

@grendello grendello grendello approved these changes

@jonpryor jonpryor Awaiting requested review from jonpryor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jpobst @jonpryor @grendello @dellis1972 @jonathanpeppers