Skip to content

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

Merged
zcbenz merged 1 commit intomasterfrom
miniak/remote-buffer
May 24, 2018
Merged

Fix support for typed arrays in remote/rpc-server#13055
zcbenz merged 1 commit intomasterfrom
miniak/remote-buffer

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 a team, codebytere and zcbenz May 24, 2018 00:04
@zcbenz
Copy link
Contributor

zcbenz commented May 24, 2018

There are tests failing related to this change.

@miniak miniak force-pushed the miniak/remote-buffer branch from 9c04740 to b2a9ebb Compare May 24, 2018 07:11
@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
Contributor

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 force-pushed the miniak/remote-buffer branch from b2a9ebb to 97bd92d Compare May 24, 2018 08:29
@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 miniak force-pushed the miniak/remote-buffer branch from 7c3dd39 to 642dc43 Compare May 24, 2018 09:38
@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
Contributor

@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.

3 participants