-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
feat: Ti.Blob#toArrayBuffer(), significant perf increase for Node.js Buffer shim #11810
Merged
Merged
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 b6fcb67
feat(android): support converting byte[] to ArrayBuffer
sgtcoolguy f2311d3
feat(android): add Ti.Blob.toArrayBuffer()
sgtcoolguy 2dd6b89
perf(buffer): make buffer shim more efficient
sgtcoolguy 98446de
test: verify Ti.Blob toArrayBuffer() and arrayBuffer() methods
sgtcoolguy 04ce8b2
test(buffer): verify buffer.copy return value
sgtcoolguy 8d2d6f9
test: enable Ti.UI.View image comparisons
sgtcoolguy 4e11ea7
docs(api): add Ti.Blob arrayBuffer/toArrayBuffer methods
sgtcoolguy File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?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
There was a problem hiding this comment.
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 anArrayBuffer
API, not anArray
API. So I could conceivably return anArrayBuffer
here instead of anArray
. 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 theByteBuffer
in place of passing abyte[]
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.
There was a problem hiding this comment.
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/arrayBufferThey use method called
arrayBuffer()
that returns aPromise
that resolves to anArrayBuffer
. 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 anArrayBuffer
(rather than the initial PR proposal oftoUint8Array()
returning anUint8Array
sync); and then add a JS level shim to add theTi.Blob.arrayBuffer()
that returns aPromise
and uses the synctoArrayBuffer()
under the hood.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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#NewDirectByteBufferI 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 directByteBuffer
to expose that to Java. On iOS, we can create anArrayBuffer
and have anNSData
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 JSArrayBuffer
as a quicker means of "dumping" bytes rather than traversing the bridge back and forth to read/write byte-by-byte. The currentTi.Buffer
is written around abyte[]
andgetBuffer()
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 aroundbyte[]
and moving toByteBuffer
is likely a fairly large change (perhaps breaking for modules).