perf: don't use JSON to send the result of `ipcRenderer.sendSync`. by tarruda · Pull Request #8953 · electron/electron

tarruda · 2017-03-17T17:21:41Z

Change the return type of AtomViewHostMsg_Message_Sync from base::string16
to base::ListValue
Adjust lib/browser/api/web-contents.js and /lib/renderer/api/ipc-renderer.js
to wrap/unwrap return values to/from array, instead of
serializing/deserializing JSON.

This change can greatly improve ipcRenderer.sendSync calls where the return
value contains Buffer instances, because those are converted to Array before
being serialized to JSON(which has no efficient way of representing byte
arrays).

A simple benchmark where remote.require('fs') was used to read a 16mb file got
at least 5x faster, not to mention it used a lot less memory. This difference
tends increases with larger buffers.

kevinsawicki · 2017-03-27T17:56:35Z

I'm wondering if this might be considered a breaking change since this changes the value returned to the renderer.

event.returnValue = {
 buffer: Buffer.from('test')
}

Before this change the renderer would receive a plain old Object back but now it receives an actual Uint8Array. It is definitely better long-term to have this more correct type returned but existing apps may break since certain APIs can't be called on a Uint8Array like Object.freeze.

This is just one possible difference, there may be others, we should probably expand test coverage of the possible sendSync return values to make sure there aren't other format changes this may be introducing.

tarruda · 2017-03-27T18:12:49Z

I'm wondering if this might be considered a breaking change since this changes the value returned to the renderer.

I could be wrong, but AFAICT this only changes the private API used by the scripts in lib/renderer/ and lib/browser. It would only break apps that use process.atomBinding to bypass the ipcRenderer public API.

kevinsawicki · 2017-03-27T18:21:56Z

I could be wrong, but AFAICT this only changes the private API used by the scripts

@tarruda try the following on master vs. this branch:

const {app, BrowserWindow, ipcMain} = require('electron')
const {Buffer} = require('buffer')

let window

app.once('ready', () => {
  window = new BrowserWindow()
  window.loadURL(`file://${__dirname}/index.html`)
})

ipcMain.on('test', (event) => {
  event.returnValue = {
    test: Buffer.from('test')
  }
})

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <script>
      const {ipcRenderer} = require('electron')
      Object.freeze(ipcRenderer.sendSync('test').test)
    </script>
  </head>
</html>

On master, no error occurs, on this branch TypeError: Cannot freeze array buffer views with elements is logged in the renderer console.

tarruda · 2017-03-27T19:04:32Z

I've only saw this change from the POV of electron.remote, which continues working since the array-like object returned by sendSync is normalized into a Buffer by that module. My mistake, sorry.

In any case, please consider these arguments in favor of merging:

It is impossible to call Object.freeze on a Buffer object in the main process, so why would someone call it on the deserialized Buffer in the renderer?
serialization of Buffer gives different results depending if the object is sent as event.returnValue or as argument to an IPC message, so we would adding more consistency to the ipc API.
Memory usage can really spike if large buffers are returned via sendSync. In the best scenario where all Buffer bytes are < 10, we would see at least 2x the number of characters in the resulting json string, but since v8 encodes strings in memory as UTF-16, we are using 4x the number of bytes in the best case scenario!
It is reasonable to expect that an object would have the same interface when deserialized, but without this patch the deserialized Buffer has no Buffer methods. It seems this PR doesn't provide only an optimization, but also fixes buffer serialization/deserialization when using sendSync 😉

kevinsawicki · 2017-03-27T19:10:31Z

In any case, please consider these arguments in favor of merging

I definitely agree this pull request improves things and should be merged eventually, I'm just worried about breaking existing apps. This is a public API and we are changing the format of the values it returns.

Without proper test coverage of this API it is unclear what other behavior might be changing since JSON.parse may have other differences than using a base::ListValue.

tarruda · 2017-03-27T19:33:30Z

👍

Maybe leave it for electron 2.0.0 then

kevinsawicki · 2017-05-22T21:37:11Z

Closing this out for now, will revisit for Electron 2.0 since this is ultimately the right direction to go for this change. 👍

ckerr · 2018-02-06T16:59:23Z

Reopening for consideration in the upcoming post-2.0 world

jkleinsc · 2018-02-06T17:05:55Z

@tarruda can you rebase this against master so that CI runs properly? Thanks!

zcbenz

Looks like a good thing to have.

MarshallOfSound · 2018-02-13T07:16:05Z

Before this is merged I'd like to see some experimentation done to see what (if any) affect this will have on sendSync or the remote API's. It would be good if we good explicitly point to uses of the API and say "this will change"

deepak1556 · 2018-02-16T13:46:35Z

atom/renderer/api/atom_api_renderer_ipc.cc

+  mate::Dictionary global(args->isolate(),
+                          args->isolate()->GetCurrentContext()->Global());
+  v8::Local<v8::Value> sandboxed;
+  if (global.GetHidden("sandboxed", &sandboxed) && sandboxed->IsTrue()) {


Would it be better to use v8::Context::[Set/Get]EmbedderData for setting up this property per context rather accessing the global object ?

ckerr · 2018-03-14T07:29:17Z

I think this would be a good candidate to land into master for 3.0.

@tarruda since we're looking at a 3.0 timeframe for this I don't think it's urgent, but could you address the comments above by @MarshallOfSound and @deepak1556?

miniak · 2018-05-28T17:01:08Z

@tarruda I've rebased the code on top of latest master after my PR #13055 got merged

miniak · 2018-05-29T15:16:00Z

There is one test failing on Windows x64, it seems like more of an issue with the test

not ok 179 BrowserWindow module "webPreferences" option "sandbox" option should set ipc event sender correctly
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
      at C:\projects\electron\spec\node_modules\mocha\lib\runnable.js:232:19

the same test is passing on Windows ia32

miniak · 2018-05-30T09:30:41Z

@zcbenz I've refactored the problematic test to make it more robust dc9e21a

miniak · 2018-05-30T09:30:58Z

@zcbenz can you please merge?

MarshallOfSound · 2018-05-30T09:36:40Z

We can have a quick discussion about this at summit, want to have a quick talk about impact

MarshallOfSound

Want to talk

miniak · 2018-05-30T10:47:24Z

@MarshallOfSound ok, what kind of impact are you concerned about?

miniak · 2018-05-30T14:55:13Z

@MarshallOfSound this PR does not change how the types are presented to JS code using the remote module, that was done in #13055. This one should only be a performance optimization.

MarshallOfSound · 2018-05-30T15:02:03Z

@miniak I've got a couple of concerns

I have a feeling there will be fine differences between the two implementations that we probably need to document. I.e. With JSON NaN becomes null, I think with this impl it will remain NaN 🤔
Because of the above, by doing this we are making the return value of sync IPC messages inconsistent with normal IPC args

I just want someone to either document the side effects of this change (differences from JSON encoding) or tell me their are none (conclusively)

miniak · 2018-05-30T16:38:59Z

@MarshallOfSound

NaN, etc. was null before, now it's being returned as undefined, which makes it consistent with how IPC arguments behave
I think that's we're actually doing the opposite, we're making the return values behave the same as the IPC arguments
is documenting in the release notes enough?

- Change the return type of AtomViewHostMsg_Message_Sync from `base::string16` to `base::ListValue` - Adjust lib/browser/api/web-contents.js and /lib/renderer/api/ipc-renderer.js to wrap/unwrap return values to/from array, instead of serializing/deserializing JSON. This change can greatly improve `ipcRenderer.sendSync` calls where the return value contains Buffer instances, because those are converted to Array before being serialized to JSON(which has no efficient way of representing byte arrays). A simple benchmark where remote.require('fs') was used to read a 16mb file got at least 5x faster, not to mention it used a lot less memory. This difference tends increases with larger buffers.

miniak · 2018-06-08T09:52:38Z

@codebytere do you maintain the release notes? How do we make sure the subtle change in behavior is documented?

miniak · 2018-06-11T11:18:26Z

@MarshallOfSound can you unblock the PR?

MarshallOfSound

Checked locally, this cleanly applies to the chromium 66 branch, approving and merging 👍

tarruda force-pushed the optimize-sendsync branch 3 times, most recently from d822ebc to 9df0602 Compare March 23, 2017 13:00

kevinsawicki self-assigned this Mar 27, 2017

kevinsawicki closed this May 22, 2017

kevinsawicki deleted the optimize-sendsync branch May 22, 2017 21:37

ckerr unassigned kevinsawicki Feb 6, 2018

ckerr restored the optimize-sendsync branch February 6, 2018 16:59

ckerr reopened this Feb 6, 2018

tarruda force-pushed the optimize-sendsync branch from 9df0602 to fb3347e Compare February 7, 2018 11:06

tarruda requested a review from a team February 7, 2018 11:06

tarruda force-pushed the optimize-sendsync branch from fb3347e to 11b9d7e Compare February 7, 2018 11:41

zcbenz approved these changes Feb 13, 2018

View reviewed changes

deepak1556 reviewed Feb 16, 2018

View reviewed changes

poiru mentioned this pull request May 24, 2018

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

Merged

miniak force-pushed the optimize-sendsync branch 2 times, most recently from 5869070 to b0e132e Compare May 28, 2018 16:59

miniak force-pushed the optimize-sendsync branch from b0e132e to 28eeea0 Compare May 28, 2018 17:09

codebytere changed the title ~~Don't use JSON to send the result of ipcRenderer.sendSync.~~ perf: don't use JSON to send the result of ipcRenderer.sendSync. May 29, 2018

miniak force-pushed the optimize-sendsync branch 2 times, most recently from 60e5e37 to dc9e21a Compare May 30, 2018 08:53

MarshallOfSound requested changes May 30, 2018

View reviewed changes

tarruda and others added 6 commits May 30, 2018 18:59

Don't base64 encode Buffers

968aac5

Don't allocate V8ValueConverter on the heap

f3674f6

Replace hidden global.sandbox with NodeBindings::IsInitialized()

9923455

Refactoring: check NodeBindings::IsInitialized() in V8ValueConverter

fc7e101

Refactor problematic test to make it more reliable

e2a7acb

miniak force-pushed the optimize-sendsync branch 2 times, most recently from 9656048 to a7601bf Compare May 30, 2018 17:00

Add tests for NaN and Infinity

7e4e0f8

miniak force-pushed the optimize-sendsync branch from a7601bf to 7e4e0f8 Compare May 30, 2018 18:33

MarshallOfSound approved these changes Jun 13, 2018

View reviewed changes

MarshallOfSound merged commit 6ff111a into master Jun 13, 2018

MarshallOfSound deleted the optimize-sendsync branch June 13, 2018 07:38

miniak mentioned this pull request Aug 5, 2018

fix: ipcRemote.sendSync() in lib/isolated_renderer/init.js #13941

Merged

4 tasks

This was referenced Sep 19, 2018

Update electron in / from 2.0.9 to 3.0.0 JetBrains/ring-ui#417

Merged

Update electron in /packages/generator/app/templates from 2.0.10 to 3.0.0 JetBrains/ring-ui#418

Merged

Conversation

tarruda commented Mar 17, 2017

Uh oh!

kevinsawicki commented Mar 27, 2017

Uh oh!

tarruda commented Mar 27, 2017

Uh oh!

kevinsawicki commented Mar 27, 2017

Uh oh!

tarruda commented Mar 27, 2017

Uh oh!

kevinsawicki commented Mar 27, 2017

Uh oh!

tarruda commented Mar 27, 2017

Uh oh!

kevinsawicki commented May 22, 2017

Uh oh!

ckerr commented Feb 6, 2018

Uh oh!

jkleinsc commented Feb 6, 2018

Uh oh!

zcbenz left a comment

Choose a reason for hiding this comment

Uh oh!

MarshallOfSound commented Feb 13, 2018

Uh oh!

deepak1556 Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

ckerr commented Mar 14, 2018

Uh oh!

miniak commented May 28, 2018

Uh oh!

miniak commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miniak commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miniak commented May 30, 2018

Uh oh!

MarshallOfSound commented May 30, 2018

Uh oh!

MarshallOfSound left a comment

Choose a reason for hiding this comment

Uh oh!

miniak commented May 30, 2018

Uh oh!

miniak commented May 30, 2018

Uh oh!

MarshallOfSound commented May 30, 2018

Uh oh!

miniak commented May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miniak commented Jun 8, 2018

Uh oh!

miniak commented Jun 11, 2018

Uh oh!

MarshallOfSound left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

miniak commented May 29, 2018 •

edited

Loading

miniak commented May 30, 2018 •

edited

Loading

miniak commented May 30, 2018 •

edited

Loading