Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

JniTypeSignature uses CultureInfo #335

Closed
jonpryor opened this issue Jun 15, 2018 · 0 comments · Fixed by #1002
Closed

JniTypeSignature uses CultureInfo #335

jonpryor opened this issue Jun 15, 2018 · 0 comments · Fixed by #1002
Labels
enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java

Comments

@jonpryor
Copy link
Member

jonpryor commented Jun 15, 2018 •
edited
Loading

(This might be an effectively meaningless optimization; presumably someone is going to initialize CultureInfo during process startup. It just so happens that in a profiler run, JniTypeSignature was the first one, hence this bug...)

The JniTypeSignature(string) constructor uses string.Contains(): https://github.com/xamarin/java.interop/blob/8ad8e1205a29b22f4b353fa3f760e5f9959712a7/src/Java.Interop/Java.Interop/JniTypeSignature.cs#L42

On Mono (and .NET?), string.Contains() delegates to CultureInfo, which requires loading and initializing the entire CultureInfo infrastructure (if it hasn't already been loaded & initialized).

As this is hitting Xamarin.Android process startup, and should be straightforward to avoid, we should fix JniTypeSignature to avoid Culture-aware string comparisons, especially as we don't need culture-aware string comparisons.

(Note: using StringComparison.Ordinal* still hits CultureInfo; it just uses CultureInfo.InvariantCulture.)
@jpobst jpobst added enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java labels Apr 16, 2020
jonpryor added a commit to jonpryor/java.interop that referenced this issue Jun 30, 2022
@jonpryor
[Java.Interop] JniTypeSignature & CultureInfo, empty strings
db93021 
Fixes: dotnet#335

Context: 920ea64

Commit 920ea64 optimized `JniTypeManager.AssertSimpleReference()` by
using `string.IndexOf(char)`.

This dovetails with Issue dotnet#335, which wanted to optimize
`JniTypeSignature` by removing calls to `string.Contains(string)`.

Fix dotnet#335, and update the `JniTypeSignature` constructor to use
`string.IndexOf(char)` instead of `string.Contains(string)`, and use
string indexers instead of `string.StartsWith()` and
`string.EndsWith()`.

`Java.Interop.dll` has no remaining usage of `string.StartsWith()`
and `string.EndsWith()`.

Additionally, update `JniTypeSignature` so that the empty string `""`
is *not* treated as a valid type signature.  This is arguably a
breaking change, but the empty string never made sense, *and* would
throw an `IndexOutOfRangeException` with `JniTypeManager.Parse()`.
@jonpryor jonpryor mentioned this issue Jun 30, 2022
Merged
jonpryor added a commit that referenced this issue Jun 30, 2022
@jonpryor
[Java.Interop] JniTypeSignature & CultureInfo, empty strings (#1002)
99c68a8 
Fixes: #335

Context: 920ea64

Commit 920ea64 optimized `JniTypeManager.AssertSimpleReference()` by
using `string.IndexOf(char)`.

This dovetails with Issue #335, which wanted to optimize
`JniTypeSignature` by removing calls to `string.Contains(string)`.

Fix #335, and update the `JniTypeSignature` constructor to use
`string.IndexOf(char)` instead of `string.Contains(string)`, and use
string indexers instead of `string.StartsWith()` and
`string.EndsWith()`.

`Java.Interop.dll` has no remaining usage of `string.StartsWith()`
and `string.EndsWith()`.

Additionally, update `JniTypeSignature` so that the empty string `""`
is *not* treated as a valid type signature.  This is arguably a
breaking change, but the empty string never made sense, *and* would
throw an `IndexOutOfRangeException` with `JniTypeManager.Parse()`.
jonpryor added a commit to dotnet/android that referenced this issue Jul 8, 2022
@jonpryor
Bump to xamarin/java.interop/main@c942ab68 (#7157)
59352f8 
Fixes: dotnet/java-interop#335
Fixes: dotnet/java-interop#954
Fixes: dotnet/java-interop#976
Fixes: dotnet/java-interop#981
Fixes: dotnet/java-interop#992

Changes: dotnet/java-interop@7716ae5...c942ab6

  * dotnet/java-interop@c942ab68: [java-source-utils] Build one `$(TargetFramework)` (#1007)
  * dotnet/java-interop@b7caa788: [Byecode] Hide `internal` Kotlin fields marked with `@JvmField` (#1004)
  * dotnet/java-interop@22d5687b: [generator] Add `@managedOverride` values `none` and `reabstract`. (#1000)
  * dotnet/java-interop@7f1d2d7a: [generator] Fix <remove-attr/> metadata (#999)
  * dotnet/java-interop@265ad762: [Java.Interop.Tools.JavaSource] Fix tag parsing errors (#997)
  * dotnet/java-interop@99c68a86: [Java.Interop] JniTypeSignature & CultureInfo, empty strings (#1002)
  * dotnet/java-interop@920ea648: [Java.Interop] optimize JniTypeManager.AssertSimpleReference() (#1001)
  * dotnet/java-interop@4b4fedd3: [generator] Process Javadoc for nested types (#996)

Ever since commit 2197a45, CI builds for the `main` branch have been
incredibly flakey: 918613d through 27647d2 all failed to complete
the **Mac** stage -- which *must* pass in order for most unit tests
to run -- then dbadf13 built, then f149c25 failed.

Nine commits in the past week have failed to adequately build.
(PR builds, meanwhile, were largely fine.)

Most of these failures are from `make all-tests`:

	_ExtractJavadocsFromJavaSourceJars:
	  /Users/runner/android-toolchain/jdk-11/bin/java -jar /Users/runner/work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar @/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmp1cf0947e.tmp 
	JAVASOURCEUTILS : warning : Invalid or corrupt jarfile /Users/runner/work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/Xamarin.Android.McwGen-Tests.csproj]
	…
	BINDINGSGENERATOR : warning BG8600: Invalid XML file '/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml': Could not find file "/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml".  For details, see verbose output. [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/Xamarin.Android.McwGen-Tests.csproj]
	  System.IO.FileNotFoundException: Could not find file "/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml"
	…
	"/Users/runner/work/1/s/xamarin-android/Xamarin.Android-Tests.sln" (default target) (1:2) ->
	"/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj" (default target) (12:6) ->
	(CoreCompile target) -> 
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(78,37): error CS1061: 'DataEventArgs' does not contain a definition for 'FromNode' and no accessible extension method 'FromNode' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(79,40): error CS1061: 'DataEventArgs' does not contain a definition for 'FromChannel' and no accessible extension method 'FromChannel' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(80,40): error CS1061: 'DataEventArgs' does not contain a definition for 'PayloadType' and no accessible extension method 'PayloadType' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(81,28): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(82,29): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(84,51): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]
	  /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(85,10): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj]

`java-source-utils.jar` is (apparently) corrupt, so
`javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml` is never created,
which means no parameter name information is present, which means the
generated `DataEventArgs` type doesn't have the appropriate property
names, as the property names come from parameter names, and since we
don't have parameter names we instead have property names like `P0`,
`P1`, `P2`, etc.

Why, then, is `java-source-utils.jar` corrupt?  The current guess is
that it is being built *concurrently*; from a
`msbuild-20220706T175333-leeroy-all.binlog` build log file from one
of the offending builds, we see:

	17:53:44.526 59:11>Target "_BuildJava: (TargetId:490)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point):
	17:53:44.526 59:11>Building target "_BuildJava" completely.
	  Output file "/Users/runner/work/1/s/xamarin-android/bin/Release/lib/packs/Microsoft.Android.Sdk.Darwin/33.0.0/tools/java-source-utils.jar" does not exist.
	…
	17:54:19.564 59:12>Target "DispatchToInnerBuilds: (TargetId:1637)" in file "/Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/sdk/7.0.100-preview.7.22354.2/Microsoft.Common.CrossTargeting.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (target "Build" depends on it):
	  Task "MSBuild" (TaskId:1099)
	    Task Parameter:BuildInParallel=True (TaskId:1099)
	    Task Parameter:Targets=Build (TaskId:1099)
	    Task Parameter:
	        Projects=
	            java-source-utils.csproj
	                    AdditionalProperties=TargetFramework=net7.0 (TaskId:1099)
	    Additional Properties for project "java-source-utils.csproj": (TaskId:1099)
	      TargetFramework=net7.0 (TaskId:1099)
	…
	17:54:19.592 59:12>Target "_BuildJava: (TargetId:1640)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point):
	  Building target "_BuildJava" completely.
	  Output file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/../../bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar" does not exist.
	…
	17:54:41.034 59:11>Done building target "_BuildJava" in project "java-source-utils.csproj".: (TargetId:490)
 
What appears to be happening -- and there is lots of conjecture here,
as the build log is enormous and it's difficult to piece everything
together -- is that `java-source-utils.csproj` is built *twice*:
Once via `Xamarin.Android.sln`, and once via `apksigner.csproj`, as
it appears that setting `%(PackageReference.AdditionalProperties)`
triggers a *nested build*; note the `DispatchToInnerBuilds` target
which invokes the `<MSBuild/>` Task with the specified
`AdditionalProperties`.

However, that's not the only potential source of concurrent builds;
`java-source-utils.csproj` used `$(TargetFrameworks)` (plural), which
can *also* result in concurrent builds between the "outer" and "inner"
builds used as part of `$(TargetFrameworks)`.

Fix this problem by bumping to dotnet/java-interop@c942ab68, which
updates `java-source-utils.csproj` to use the singular
`$(TargetFramework)` property -- avoiding "outer" and "inner" build
problems -- and "for good measure" update `apksigner.csproj` to
remove `%(ProjectReference.AdditionalProperties)` on
`java-source-utils.csproj`.  This should ensure that
`java-source-utils.csproj` is only built *once*, which in turn should
ensure that `java-source-utils.jar` isn't corrupted, and our CI
builds become more reliable
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Assignees
No one assigned
Labels
enhancement Proposed change to current functionality java-interop Runtime bridge between .NET and Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Java.Interop] JniTypeSignature & CultureInfo, empty strings jonpryor/java.interop
2 participants
@jonpryor @jpobst