Skip to content

Conversation

@jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Oct 20, 2017

This ensures the following values are set on Windows:

  • MONO_TRACE_LISTENER
  • JAVA_INTEROP_GREF_LOG
  • JAVA_INTEROP_LREF_LOG

This also cleans up what needs to be done downstream in xamarin-android:
dotnet/android#949

This required a new Java.Interop.BootstrapTasks project
that adds a SetEnvironmentVariable MSBuild task.
This will be useful down the road, for setting up "prepare"
steps in this repo for Windows.

Mono 5.8 also seems to have introduced a slowness in the
PerformanceTests, included a fix to allow more digits in
the time elapsed. This prevents a test failure on Mono 5.8.

Also minor updates to .gitignore file.

@dnfclas
Copy link

dnfclas commented Oct 20, 2017

@jonathanpeppers,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@jonpryor
Copy link
Contributor

This PR requires using msbuild, as <UsingTask/> isn't implemented on xbuild.

Which is why the PR build says:

Build finished. No test results found.

...which is problematic, actually, because everything is failing:

	Target RunTests:
: error : Error initializing task SetEnvironmentVariable: Not registered task SetEnvironmentVariable.
		Build FAILED.

I wonder if the xbuild exit value is 0, and that's why nothing "bad" happens...

Best case, the <UsingTask/> and related code needs to be made conditional on Windows use. Worst case, this isn't going to work, not as long as we're supporting xbuild.

@jonathanpeppers jonathanpeppers force-pushed the windows-env-vars branch 3 times, most recently from dd8d833 to cf385eb Compare October 23, 2017 16:41
@jonathanpeppers
Copy link
Member Author

@jonpryor
Copy link
Contributor

The error:

System.ArgumentOutOfRangeException : Specified argument was out of the range of valid values.
Parameter name: count
+++++++++++++++++++
STACK TRACE:
at System.String.CreateString (System.Char c, System.Int32 count) [0x00004] in <4656b2b94f914437bce672312dd9e44b>:0
at (wrapper managed-to-managed) System.String..ctor(char,int)
at Java.Interop.PerformanceTests.JniMethodInvocationOverheadTiming.FormatFraction (System.Double value, System.Int32 width, System.Int32 fractionWidth) [0x0002d] in <df0a6bca98204d318b5ed1cb9471bab2>:0
at Java.Interop.PerformanceTests.JniMethodInvocationOverheadTiming.MethodInvocationTiming () [0x00820] in <df0a6bca98204d318b5ed1cb9471bab2>:0

The code:

static string FormatFraction (double value, int width, int fractionWidth)
{
	var v = value.ToString ("0.0" + new string ('#', fractionWidth - 1));
	var i = v.IndexOf (NumberFormatInfo.CurrentInfo.NumberDecimalSeparator);
	var p = new string (' ', width - fractionWidth - i - 1);
	return p + v + new string (' ', width - p.Length - v.Length);
}

Presumably fractionWidth is <= 0, or width - fractionWidth - i - 1 is negative.

Are you able to repro locally?

@jonathanpeppers
Copy link
Member Author

It seems to work fine for me locally:

$ mono packages/NUnit.Runners.2.6.3/tools/nunit-console.exe bin/TestDebug/Java.Interop-PerformanceTests.dll
...
Tests run: 8, Errors: 0, Failures: 0, Inconclusive: 0, Time: 2.4159998 seconds
  Not run: 0, Invalid: 0, Ignored: 0, Skipped: 0

I have this mono:

Mono JIT compiler version 5.4.0.201 (2017-06/71277e78f6e Thu Sep 21 19:22:55 EDT 2017)

@jonathanpeppers
Copy link
Member Author

@jonpryor some insight on the test failure.

I installed Mono 5.8 and the performance tests are now running pretty slow.

A previous test on Jenkins took about 40 sec total. That same test project is now taking over 6 min when I run it locally!

The values passed to FormatFraction are (192780.4471, 10, 5).

@jonathanpeppers
Copy link
Member Author

@akoeplinger do you know the right person to check into the slowness of these tests?

I'm going to fix a bug in the test to fix a case if there are too many digits in the time elapsed, but it still looks like there is something going on with Mono 5.8.

@akoeplinger
Copy link
Member

@jonathanpeppers yes, it's me 😄 I'll try to run these tests locally and see what I can find.

This ensures the following values are set on Windows:
- MONO_TRACE_LISTENER
- JAVA_INTEROP_GREF_LOG
- JAVA_INTEROP_LREF_LOG

This also cleans up what needs to be done downstream in xamarin-android:
dotnet/android#949

This required a new `Java.Interop.BootstrapTasks` project
that adds a `SetEnvironmentVariable` MSBuild task.
This will be useful down the road, for setting "prepare"
steps in this repo for Windows.

Mono 5.8 also seems to have introduced a slowness in the
PerformanceTests, included a fix to allow more digits in
the time elapsed. This prevents a test failure on Mono 5.8.

Also minor updates to `.gitignore` file.
@akoeplinger
Copy link
Member

As discussed over Slack, Mono 5.8 doesn't seem to be the issue (at least we can't repro the slowdown locally).

@jonpryor jonpryor merged commit 5cf61f5 into dotnet:master Oct 25, 2017
Redth pushed a commit to Redth/java.interop that referenced this pull request Oct 26, 2017
This ensures the following values are set on Windows:

  - MONO_TRACE_LISTENER
  - JAVA_INTEROP_GREF_LOG
  - JAVA_INTEROP_LREF_LOG

This also cleans up what needs to be done downstream in xamarin-android:
dotnet/android#949

This required a new `Java.Interop.BootstrapTasks` project
that adds a `<SetEnvironmentVariable/>` MSBuild task.
This will be useful down the road, for setting "prepare"
steps in this repo for Windows.

Running `PerformanceTests` also is occasionally, *randomly*, slower
than expected. (The exact reason why is currently unknown.) When it's
"too" slow, an `ArgumentOutOfRangeException` was being thrown from
`FormatFraction()` because the overall width for the time was longer
than what was permitted. Increase the width to allow more digits.

Minor updates to `.gitignore` file.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 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