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.

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

Fix support for typed arrays in remote/rpc-server #13055

Merged
merged 1 commit into from May 24, 2018
Merged

Conversation

miniak
Copy link
Contributor

@miniak miniak commented May 24, 2018

Adds support for ArrayBuffer and all typed array views (except for DataView) in the remote module.

Also makes the IPC payload much smaller (in the net module tests the payload goes down from approx 13 KB to 5 KB) by using base64 encoding instead of an array of values.

for example when calling electron.remote.getCurrentWindow().getNativeWindowHandle(); this is the JSON with the return value

before:
[{"type":"buffer","value":{"type":"Buffer","data":[32,72,27,0,64,96,0,0]}}]
after:
[{"type":"buffer","value":{"type":"Buffer","data":"oAzG+9B/AAA="}}]

Also calling Buffer.from(value) instead of Buffer.from(value.buffer) would create an unnecessary copy of the data instead of just creating a different view of the underlying memory.

https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_buffer

Copies the passed buffer data onto a new Buffer instance.

https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_arraybuffer_byteoffset_length

This creates a view of the ArrayBuffer without copying the underlying memory. For example, when passed a reference to the .buffer property of a TypedArray instance, the newly created Buffer will share the same allocated memory as the TypedArray.

@miniak miniak requested review from zcbenz, codebytere and a team May 24, 2018 00:04
@zcbenz
Copy link
Member

zcbenz commented May 24, 2018

There are tests failing related to this change.

@miniak
Copy link
Contributor Author

miniak commented May 24, 2018

@zcbenz should be fixed now

@miniak
Copy link
Contributor Author

miniak commented May 24, 2018

@zcbenz I still have issue with supports TypedArray in api-remote-spec.js. Actually it seems like Int16Array etc is not passed properly without my change, example:

var a = new Int16Array([256, 512, 768, 1024])
a -> Int16Array(4) [256, 512, 768, 1024]
Buffer.from(a) -> [0, 0, 0, 0]

so the test does not even seem to be correct. Just try changing const int16values = new Int16Array([1, 2, 3, 4]) to const int16values = new Int16Array([256, 512, 768, 1024])

you'll get this error

AssertionError [ERR_ASSERTION]: <Buffer 00 00 00 00> deepEqual Int16Array [ 256, 512, 768, 1024 ]

@miniak
Copy link
Contributor Author

miniak commented May 24, 2018

@zcbenz I would suggest adding full support for all typed array types in a separate PR, I am willing to do that.

@zcbenz
Copy link
Member

zcbenz commented May 24, 2018

@zcbenz I would suggest adding full support for all typed array types in a separate PR, I am willing to do that.

Sounds good to me 👍

@miniak miniak changed the title Serialize buffer as base64 string in remote/rpc-server Add proper support for typed arrays in remote/rpc-server May 24, 2018
@miniak
Copy link
Contributor Author

miniak commented May 24, 2018

@zcbenz I've changed my mind, let's do it in this PR. It's quite simple

@miniak miniak force-pushed the miniak/remote-buffer branch 6 times, most recently from ca16d8a to 7c3dd39 Compare May 24, 2018 09:34
@miniak
Copy link
Contributor Author

miniak commented May 24, 2018

@zcbenz I think I'm done with the changes. Can you please review?

@miniak miniak changed the title Add proper support for typed arrays in remote/rpc-server Fix support for typed arrays in remote/rpc-server May 24, 2018
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

👍

@zcbenz zcbenz merged commit 4cfe5ec into master May 24, 2018
@zcbenz zcbenz deleted the miniak/remote-buffer branch May 24, 2018 12:05
@poiru
Copy link
Contributor

poiru commented May 24, 2018

See also #8953, cc @tarruda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants