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

feat: Ti.Blob#toArrayBuffer(), significant perf increase for Node.js Buffer shim #11810

Merged
merged 8 commits into from
Aug 13, 2020
Merged

feat: Ti.Blob#toArrayBuffer(), significant perf increase for Node.js Buffer shim #11810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
98b8ac5
feat(ios): add Ti.Blob.toArrayBuffer()
sgtcoolguy Jul 7, 2020
b6fcb67
feat(android): support converting byte[] to ArrayBuffer
sgtcoolguy Jul 28, 2020
f2311d3
feat(android): add Ti.Blob.toArrayBuffer()
sgtcoolguy Jul 28, 2020
2dd6b89
perf(buffer): make buffer shim more efficient
sgtcoolguy Jul 28, 2020
98446de
test: verify Ti.Blob toArrayBuffer() and arrayBuffer() methods
sgtcoolguy Jul 28, 2020
04ce8b2
test(buffer): verify buffer.copy return value
sgtcoolguy Jul 28, 2020
8d2d6f9
test: enable Ti.UI.View image comparisons
sgtcoolguy Jul 28, 2020
4e11ea7
docs(api): add Ti.Blob arrayBuffer/toArrayBuffer methods
sgtcoolguy Jul 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions ...kroll-apt/src/main/resources/org/appcelerator/kroll/annotations/generator/ProxyBinding.fm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,18 @@ namespace ${ns?lower_case} {
"javaDeleteLocalRef":true,
"defaultValue": "null"
},
"byte[]":{
"jsType":"ArrayBuffer",
"jsHandleCast":"ArrayBuffer",
"jsToJavaConverter":"jsArrayBufferToJavaByteArray",
"javaToJsConverter":"javaByteArrayToJsArrayBuffer",
"jvalue":"l", "signature":"[B",
"javaCallMethodType":"Object",
"javaReturnType":"jbyteArray",
"javaValidation":true,
"javaDeleteLocalRef":true,
"defaultValue": "null"
},
"int[]":{
"jsType":"Array",
"jsHandleCast":"Array",
Expand Down
2 changes: 2 additions & 0 deletions android/runtime/v8/src/native/JNIUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ jclass JNIUtil::doubleClass = NULL;
jclass JNIUtil::booleanClass = NULL;
jclass JNIUtil::stringArrayClass = NULL;
jclass JNIUtil::objectArrayClass = NULL;
jclass JNIUtil::byteArrayClass = NULL;
jclass JNIUtil::shortArrayClass = NULL;
jclass JNIUtil::intArrayClass = NULL;
jclass JNIUtil::longArrayClass = NULL;
Expand Down Expand Up @@ -312,6 +313,7 @@ void JNIUtil::initCache()
floatClass = findClass("java/lang/Float");
doubleClass = findClass("java/lang/Double");
booleanClass = findClass("java/lang/Boolean");
byteArrayClass = findClass("[B");
shortArrayClass = findClass("[S");
intArrayClass = findClass("[I");
longArrayClass = findClass("[J");
Expand Down
1 change: 1 addition & 0 deletions android/runtime/v8/src/native/JNIUtil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class JNIUtil
static jclass booleanClass;
static jclass stringArrayClass;
static jclass objectArrayClass;
static jclass byteArrayClass;
static jclass shortArrayClass;
static jclass intArrayClass;
static jclass longArrayClass;
Expand Down
25 changes: 25 additions & 0 deletions android/runtime/v8/src/native/TypeConverter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,28 @@ Local<Array> TypeConverter::javaArrayToJsArray(Isolate* isolate, JNIEnv *env, ji
return jsArray;
}

Local<ArrayBuffer> TypeConverter::javaByteArrayToJsArrayBuffer(Isolate* isolate, jbyteArray javaByteArray)
{
JNIEnv *env = JNIScope::getEnv();
if (env == NULL) {
return Local<ArrayBuffer>();
}
return TypeConverter::javaByteArrayToJsArrayBuffer(isolate, env, javaByteArray);
}

Local<ArrayBuffer> TypeConverter::javaByteArrayToJsArrayBuffer(Isolate* isolate, JNIEnv *env, jbyteArray javaByteArray)
{
size_t byteCount = env->GetArrayLength(javaByteArray);
Local<ArrayBuffer> jsArray = ArrayBuffer::New(isolate, byteCount);
if (byteCount > 0) {
void* sourcePointer = (void*)(env->GetByteArrayElements(javaByteArray, 0));
void* destinationPointer = jsArray->GetContents().Data();
memcpy(destinationPointer, sourcePointer, byteCount);
env->ReleaseByteArrayElements(javaByteArray, reinterpret_cast<jbyte*>(sourcePointer), JNI_ABORT);
}
return jsArray;
Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at our "v8.h" header file. We should be able to use memcpy() to copy the bytes over which should be significantly faster. Especially for large arrays. Would you mind giving this a go please?

size_t byteCount = env->GetArrayLength(javaByteArray);
Local<Array> jsArray = Array::New(isolate, byteCount);
if (byteCount > 0) {
	void* sourcePointer = (void*)(env->GetByteArrayElements(javaByteArray, 0));
	void* destinationPointer = jsArray->GetContents().Data();
	memcpy(destinationPointer, sourcePointer, byteCount);
}
return jsArray;

An even better way of doing this is to "pin" the Java byte array into memory and wrap it with the V8 ArrayBuffer via the Array::New(Isolate*, void*, size_t) API... but... unfortunately... Android JNI does not guarantee it will pin it in memory and may make a copy instead. So, this idea is out. (Bummer.)
https://developer.android.com/training/articles/perf-jni#primitive-arrays

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 don't think this will work, because the GetContents().Data() is an ArrayBuffer API, not an Array API. So I could conceivably return an ArrayBuffer here instead of an Array. I alluded to it in the PR body, but basically the most efficient mechanism here is to make use of ByteBuffer in java and shared the underlying memory with the Uint8Array/TypedArray in the JS engine. But that'd require some changes to our Java APIs to use the ByteBuffer in place of passing a byte[] around.

I'll see if I can't just swap to have it return an ArrayBuffer under the hood. It'll require tweaks in other places, but that may be a better fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so looks like the Web Blob actually has an API for this! https://developer.mozilla.org/en-US/docs/Web/API/Blob/arrayBuffer

They use method called arrayBuffer() that returns a Promise that resolves to an ArrayBuffer. Given that, it probably makes sense to try and align here. The obvious issue right now is our lack of native Promise APIs (see #10554)

So in the interim, perhaps a Ti.Blob.toArrayBuffer() which directly/synchronously returns an ArrayBuffer (rather than the initial PR proposal of toUint8Array() returning an Uint8Array sync); and then add a JS level shim to add the Ti.Blob.arrayBuffer() that returns a Promise and uses the sync toArrayBuffer() under the hood.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could still try to make use of pinned array elements?

size_t byteCount = env->GetArrayLength(javaByteArray);
if (byteCount == 0) {
  return ArrayBuffer::New(isolate, 0);
}

jboolean isCopy = JNI_FALSE;
void* src = (void*)(env->GetByteArrayElements(javaByteArray, &isCopy));

if (isCopy == JNI_FALSE) {
  return ArrayBuffer::New(isolate, src, byteCount);
}

Local<Array> jsArray = ArrayBuffer::New(isolate, byteCount);
void* dst = jsArray->GetContents().Data();
memcpy(dst, src, byteCount);
return jsArray;

Copy link
Contributor

Choose a reason for hiding this comment

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

First, the code I posted is missing a call to ReleaseByteArrayElements(). This is becauseGetByteArrayElements() increments the reference count. So, we must call the release method to decrement it and avoid a memory leak.

If we were to take advantage of "pinning", then we shouldn't release the Java byte array reference until the V8 array object has been garbage collected.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible to use GetPrimitiveArrayCritical to guarantee pinned access. However, using this locks the GC while the array is being used.

It looks like ART implements a different behaviour, where GC is still possible but compacting GCs that would move the array are not.
https://android.googlesource.com/platform/art/+/refs/heads/master/runtime/jni/jni_internal.cc#2099
https://android.googlesource.com/platform/art/+/refs/heads/master/runtime/gc/heap.cc#2073

Similarly, GetStringCritical also exists.
https://android.googlesource.com/platform/art/+/refs/heads/master/runtime/jni/jni_internal.cc#1907

Maybe we could experiment with this?

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 think the correct long-term move here it to use ByteBuffer and https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html#NewDirectByteBuffer

I opened https://jira.appcelerator.org/browse/TIMOB-28039 to track this.

Basically the underlying ArrayBuffer in the engine can give us a pointer to the memory and we can allocate a direct ByteBuffer to expose that to Java. On iOS, we can create an ArrayBuffer and have an NSData point at the same memory pool.

So for this PR, I'm really trying not to break our APIs but just do a copy of the byte[] to generate a JS ArrayBuffer as a quicker means of "dumping" bytes rather than traversing the bridge back and forth to read/write byte-by-byte. The current Ti.Buffer is written around a byte[] and getBuffer() is a "module" api.

But I definitely think we can use memory sharing long-term to underpin Ti.Buffer and avoid traversing the bridge at all for reads/writes and still have both the native and JS sides referring to the same bytes. But it is likely a larger change than I wanted to get into here, hence why I broke off a separate ticket. The code as-is makes some assumptions about the buffer being a thing wrapper around byte[] and moving to ByteBuffer is likely a fairly large change (perhaps breaking for modules).

}

jlongArray TypeConverter::jsArrayToJavaLongArray(Isolate* isolate, Local<Array> jsArray)
{
JNIEnv *env = JNIScope::getEnv();
Expand Down Expand Up @@ -1033,6 +1055,9 @@ Local<Value> TypeConverter::javaObjectToJsValue(Isolate* isolate, JNIEnv *env, j
} else if (env->IsInstanceOf(javaObject, JNIUtil::objectArrayClass)) {
return javaArrayToJsArray(isolate, (jobjectArray) javaObject);

} else if (env->IsInstanceOf(javaObject, JNIUtil::byteArrayClass)) {
return javaByteArrayToJsArrayBuffer(isolate, (jbyteArray) javaObject);

} else if (env->IsInstanceOf(javaObject, JNIUtil::shortArrayClass)) {
return javaArrayToJsArray(isolate, (jshortArray) javaObject);

Expand Down
2 changes: 2 additions & 0 deletions android/runtime/v8/src/native/TypeConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ class TypeConverter
static v8::Local<v8::Array> javaArrayToJsArray(v8::Isolate* isolate, jbooleanArray javaBooleanArray);
static jshortArray jsArrayToJavaShortArray(v8::Isolate* isolate, v8::Local<v8::Array> jsArray);
static v8::Local<v8::Array> javaArrayToJsArray(v8::Isolate* isolate, jshortArray javaShortArray);
static v8::Local<v8::ArrayBuffer> javaByteArrayToJsArrayBuffer(v8::Isolate* isolate, jbyteArray javaByteArray);
static jintArray jsArrayToJavaIntArray(v8::Isolate* isolate, v8::Local<v8::Array> jsArray);
static v8::Local<v8::Array> javaArrayToJsArray(v8::Isolate* isolate, jintArray javaIntArray);
static jlongArray jsArrayToJavaLongArray(v8::Isolate* isolate, v8::Local<v8::Array> jsArray);
Expand All @@ -154,6 +155,7 @@ class TypeConverter
static jarray jsArrayToJavaArray(v8::Isolate* isolate, JNIEnv *env, v8::Local<v8::Array> jsArray);
static jobjectArray jsArrayToJavaStringArray(v8::Isolate* isolate, JNIEnv *env, v8::Local<v8::Array> jsArray);
static v8::Local<v8::Array> javaArrayToJsArray(v8::Isolate* isolate, JNIEnv *env, jbooleanArray javaBooleanArray);
static v8::Local<v8::ArrayBuffer> javaByteArrayToJsArrayBuffer(v8::Isolate* isolate, JNIEnv *env, jbyteArray javaByteArray);
static jshortArray jsArrayToJavaShortArray(v8::Isolate* isolate, JNIEnv *env, v8::Local<v8::Array> jsArray);
static v8::Local<v8::Array> javaArrayToJsArray(v8::Isolate* isolate, JNIEnv *env, jshortArray javaShortArray);
static jintArray jsArrayToJavaIntArray(v8::Isolate* isolate, JNIEnv *env, v8::Local<v8::Array> jsArray);
Expand Down
3 changes: 2 additions & 1 deletion android/titanium/src/java/org/appcelerator/titanium/TiBlob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* Appcelerator Titanium Mobile
* Copyright (c) 2009-2013 by Appcelerator, Inc. All Rights Reserved.
* Copyright (c) 2009-2020 by Appcelerator, Inc. All Rights Reserved.
* Licensed under the terms of the Apache Public License
* Please see the LICENSE included with this distribution for details.
*/
Expand Down Expand Up @@ -292,6 +292,7 @@ public void loadBitmapInfo()
* @return binary data.
* @module.api
*/
@Kroll.method(name = "toArrayBuffer")
public byte[] getBytes()
{
byte[] bytes = new byte[0];
Expand Down
12 changes: 10 additions & 2 deletions apidoc/Titanium/Blob.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ properties:
permission: read-only

methods:

- name: toString
returns:
type: String
Expand Down Expand Up @@ -188,7 +187,6 @@ methods:
type: Titanium.Blob
summary: The image thumbnail in a blob.
parameters:

- name: size
type: Number
summary: Size of the thumbnail, in either width or height.
Expand Down Expand Up @@ -250,3 +248,13 @@ methods:
returns:
type: Titanium.Blob
summary: The image with a transparent border in a blob, or `null` if this blob is not an image.

- name: toArrayBuffer
returns:
type: ArrayBuffer
summary: Returns an `ArrayBuffer` representation of this blob.

- name: arrayBuffer
returns:
type: Promise<ArrayBuffer>
summary: Returns a `Promise` that resolves with the contents of the blob as binary data contained in an `ArrayBuffer`.