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

Conversation

@joaomoreno
Copy link
Member

commented Aug 20, 2018

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

@joaomoreno joaomoreno added this to the August 2018 milestone Aug 20, 2018

@joaomoreno joaomoreno self-assigned this Aug 20, 2018

@joaomoreno joaomoreno requested a review from sandy081 Aug 20, 2018

@joaomoreno

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2018

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: 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

This comment has been minimized.

Copy link
Member Author

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 added some commits Aug 20, 2018

@joaomoreno joaomoreno merged commit d259a97 into master Aug 21, 2018

2 checks passed

VSTS: VS Code 20180821.9 succeeded
Details
license/cla All CLA requirements met.
Details

@joaomoreno joaomoreno deleted the joao/ipc-buffers branch Aug 21, 2018

@bpasero

This comment has been minimized.

Copy link
Member

commented on src/vs/base/parts/ipc/common/ipc.electron.ts in e512c12 Dec 10, 2018

@joaomoreno Electron 3.0.x landed in master now if you want to check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.