Bug 31260 - The binding for Java.Nio.ByteBuffer.Get(byte[], int, int) allocates a new temporary Java byte array on every call
Summary: The binding for Java.Nio.ByteBuffer.Get(byte[], int, int) allocates a new tem...
Status: CONFIRMED
Alias: None
Product: Android
Classification: Xamarin
Component: BCL Class Libraries ()
Version: 5.1
Hardware: PC Mac OS
: Normal enhancement
Target Milestone: ---
Assignee: dean.ellis
URL:
Depends on:
Blocks:
 
Reported: 2015-06-19 16:20 UTC by Brendan Zagaeski (Xamarin Team, assistant)
Modified: 2015-06-26 00:17 UTC (History)
1 user (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Test case (30.44 KB, application/zip)
2015-06-19 16:20 UTC, Brendan Zagaeski (Xamarin Team, assistant)
Details
Example logcat output (3.59 KB, text/plain)
2015-06-19 16:21 UTC, Brendan Zagaeski (Xamarin Team, assistant)
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report for Bug 31260 on Developer Community or GitHub if you have new information to add and do not yet see a matching new report.

If the latest results still closely match this report, you can use the original description:

  • Export the original title and description: Developer Community HTML or GitHub Markdown
  • Copy the title and description into the new report. Adjust them to be up-to-date if needed.
  • Add your new information.

In special cases on GitHub you might also want the comments: GitHub Markdown with public comments

Related Links:
Status:
CONFIRMED

Description Brendan Zagaeski (Xamarin Team, assistant) 2015-06-19 16:20:29 UTC
Created attachment 11689 [details]
Test case

The binding for Java.Nio.ByteBuffer.Get(byte[], int, int) allocates a new temporary Java byte array on every call




## Regression status: not a regression

The behavior is the same on Xamarin.Android 5.1.4 and Xamarin.Android 4.20.1.




## Motivation

The motivation for this bug is scenarios where apps call `ByteBuffer.Get()` many times in a loop. One place where this can arise is when streaming audio via `MediaCodec` and `AudioTrack` (see for example [1]). With the current binding, this causes many allocations of new intermediate Java byte arrays, which can be a performance problem (see also [2]).


> [1] https://forums.xamarin.com/discussion/33840/best-way-to-stream-data-from-mediacodec-to-audiotrack
> [2] https://forums.xamarin.com/discussion/28763/efficient-conversion-of-bytearray-to-byte




## Steps to reproduce


1. Run the attached test case on an Android 4.3 or lower emulator or device.


2. Tap the "Hello World, Click Me!" button several times.


3. Check the logcat output.




## Results

Each button click invokes `Java.Nio.ByteBuffer.Get(byte[] dst, int dstOffset, int byteCount)`.


The C# binding of the `Get()` method has the following steps:

a. Call `JNIEnv.NewArray(dst)` to create an intermediate Java byte array from the C# `dst` array.

b. Call the underlying Java `java.nio.ByteBuffer.get()` method via JNI, passing in the Java byte array.

c. Copy the bytes back out of the Java byte array into the C# `dst` byte array.



This is an efficiency problem due to step (a). Allocating a new Java byte array and copying the bytes into it from the C# `dst` array incurs a significant performance overhead compared to a pure Java app.


Part of this performance problem can be observed via the logcat output. Depending on the device and the size of the `bytes` byte array, each button click can potentially cause a "GC_CONCURRENT" garbage collection:

> 06-18 15:12:23.722 D/dalvikvm( 3029): GC_FOR_ALLOC freed 6K, 5% free 3125K/3288K, paused 2ms, total 2ms
> 06-18 15:12:23.722 I/dalvikvm-heap( 3029): Grow heap (frag case) to 4.103MB for 1048588-byte allocation
> 06-18 15:12:23.726 D/dalvikvm( 3029): GC_FOR_ALLOC freed <1K, 4% free 4148K/4316K, paused 4ms, total 4ms
> 06-18 15:12:23.730 D/dalvikvm( 3029): GC_CONCURRENT freed 0K, 4% free 4148K/4316K, paused 2ms+0ms, total 3ms




## Possible partial workaround using `Marshal.Copy()`

As discussed on [2], one possible way work around this issue is to skip the creation of the intermediate Java byte array and instead copy the bytes directly from the Java ByteBuffer to the C# byte array using `Marshal.Copy()`. This requires creating the ByteBuffer via `AllocateDirect()` to ensure that a contiguous block of bytes is used (see also [3]).

> [3] http://stackoverflow.com/questions/5670862/bytebuffer-allocate-vs-bytebuffer-allocatedirect




## Possible partial workaround by creating a new overload of `Get()` that takes a Java byte array

Depending on the desired use of the bytes retrieved from `ByteBuffer.Get()`, one way to avoid the performance problem would be to pass a Java byte array into a modified version of the `Get()` method.


For example... 


1. Create an overload of the `Get()` method that takes a `JavaArray<Java.Lang.Byte>` rather than a `byte[]`. One way to do this is to add an extension method (remove the > greater-than symbol from the beginning of each line):

> public static class Extensions
> {
>     static IntPtr byteBufferClassRef;
>     static IntPtr byteBufferGetBII;
> 
>     public static ByteBuffer Get(this ByteBuffer buffer, JavaArray<Java.Lang.Byte> dst, int dstOffset, int byteCount)
>     {
>         if (byteBufferClassRef == IntPtr.Zero) {
>             byteBufferClassRef = JNIEnv.FindClass("java/nio/ByteBuffer");
>         }
>         if (byteBufferGetBII == IntPtr.Zero) {
>             byteBufferGetBII = JNIEnv.GetMethodID (byteBufferClassRef, "get", "([BII)Ljava/nio/ByteBuffer;");
>         }
> 
>         return Java.Lang.Object.GetObject<ByteBuffer>(JNIEnv.CallObjectMethod(buffer.Handle, byteBufferGetBII, new JValue[] {
>             new JValue(dst),
>             new JValue(dstOffset),
>             new JValue(byteCount)
>         }), JniHandleOwnership.TransferLocalRef);
>     }
> }


2. Call this new overload of `Get()`, passing in a `JavaArray<Java.Lang.Byte>`.


3. After the call to `Get()` completes, you can either use the `JavaArray<Java.Lang.Byte>` object directly in another Java method [4], or you can optionally copy the contents back into a C# byte array:

> JNIEnv.CopyArray(myJavaByteArray.Handle, myCsharpByteArray);


[4] Depending how the Java method is bound in Xamarin, this might require creating another extension method.




Even if you call `JNIEnv.CopyArray()` after _every_ call to the modified version of `Get()`, this workaround still has a performance advantage over the original version of `Get()`: it allows you to perform exactly _one_ allocation of a new `JavaArray<Java.Lang.Byte>`, and then _re-use_ that array every time you call the `Get()` method.



## Additional notes to the Xamarin team

Maybe it would be worth adding the `Get(JavaArray<Java.Lang.Byte>, int, int)` overload of this binding to Xamarin.Android to give users easier flexibility in choosing how and when to copy data from Java to C# for this somewhat "low-level" method?

I am not certain if this issue would best be classified as a bug or an enhancement. Feel free to re-categorize it if needed.




## Test environment


### Devices

- Xamarin Android Player 0.3.7 (1), Nexus 4 (KitKat)

- LG Optimus L9, Android 4.1.2 (API 16)


### OS X 10.10.3

=== Xamarin.Android ===

Version: 5.1.4.8 (Enterprise Edition)
Android SDK: /Users/macuser/Library/Developer/Xamarin/android-sdk-macosx
	Supported Android versions:
		2.3    (API level 10)
		4.0.3  (API level 15)
		4.1    (API level 16)
		4.2    (API level 17)
		4.3    (API level 18)
		4.4    (API level 19)
		4.4.87 (API level 20)
		5.0    (API level 21)
Java SDK: /usr
java version "1.8.0"
Java(TM) SE Runtime Environment (build 1.8.0-b132)
Java HotSpot(TM) 64-Bit Server VM (build 25.0-b70, mixed mode)
Comment 1 Brendan Zagaeski (Xamarin Team, assistant) 2015-06-19 16:21:17 UTC
Created attachment 11690 [details]
Example logcat output
Comment 2 Jonathan Pryor 2015-06-19 16:31:12 UTC
This is an issue with the binding.

If we were free to break ABI, at this point I'd replace all arrays with JavaArray<T>/etc. use.

Unfortunately, that would be a huge break. ;-)

We could update the generator to emit overloads. The problem with that is twofold:

1. It would hugely increase the size of the (already gigantic) binding, and
2. It doesn't help with interfaces. (Though this might be fixable?)

We should try to figure out how bad (1) is.

Concurrently, we should see if there is a set of methods that we should do this manually for, as opposed to doing it for "all" array-using methods.
Comment 3 Brendan Zagaeski (Xamarin Team, assistant) 2015-06-26 00:17:18 UTC
## Additional note for the `JavaArray<Java.Lang.Byte>` overload workaround

In case any users come across this page while researching this issue and would like to try the workaround of creating a new `Get()` overload, here is a way to create an instance of `JavaArray<Java.Lang.Byte>` to pass into the new method (remove the > greater-than symbols from the beginning of each line):

> byte[] bytes = new byte[1024];
> IntPtr javaBytesLref = JNIEnv.NewArray(bytes);
> JavaArray<Java.Lang.Byte> bytesJava = JavaArray<Java.Lang.Byte>.FromJniHandle(javaBytesLref, JniHandleOwnership.TransferLocalRef);