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

IPC protocol should be buffer based, not JSON based #56820

Merged
merged 5 commits into from
Aug 21, 2018
Merged

Conversation

joaomoreno
Copy link
Member

@joaomoreno joaomoreno commented Aug 20, 2018

  • Make IPC Buffer based, instead of any based
  • Provide better buffer serialization for buffer data arguments

@joaomoreno joaomoreno added feature-request Request for new features or functionality ipc labels Aug 20, 2018
@joaomoreno joaomoreno added this to the August 2018 milestone Aug 20, 2018
@joaomoreno joaomoreno self-assigned this Aug 20, 2018
@joaomoreno
Copy link
Member Author

Pushed the first step. This allows us full control at the protocol layer as to how to serialize/deserialize things; namely, if that thing is a buffer, everything's easier.

One issue emerged. Because the abstraction is one layer below, we possibly lost performance on two areas:

  • IPC over child processes using fork (ipc.cp.ts)
  • IPC over Electron

Both these implementations do not support Buffers currently. Here's my proposal for each:

  • Remove protocol work from ipc.cp.ts and make it dependent on ipc.net.ts, since that implementation works using named pipes and sockets. This will let us be independent from node's fork and serialization, which is a good thing anyway.
  • Wait for Electron 3 which brings Buffer support over IPC: Fix support for typed arrays in remote/rpc-server electron/electron#13055. This is fine since we send very small and few messages over Electron IPC. Another alternative here is to also ditch Electron IPC and just use ipc.net.ts as well.

@joaomoreno
Copy link
Member Author

joaomoreno commented Aug 20, 2018

Second step pushed. Buffers can now be used in the following places:

  • Promise request arg
  • Promise resolution result
  • Event request arg
  • Event data

@microsoft microsoft deleted a comment from Ngocbich95 Aug 20, 2018
@joaomoreno joaomoreno merged commit d259a97 into master Aug 21, 2018
@joaomoreno joaomoreno deleted the joao/ipc-buffers branch August 21, 2018 08:00
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants