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

perf: don't use JSON to send the result of `ipcRenderer.sendSync`. #8953

Merged
merged 7 commits into from Jun 13, 2018

Conversation

Projects
None yet
8 participants
@tarruda
Contributor

tarruda commented Mar 17, 2017

  • 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 kevinsawicki self-assigned this Mar 27, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Mar 27, 2017

Contributor

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.

Contributor

kevinsawicki commented Mar 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 27, 2017

Contributor

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.

Contributor

tarruda commented Mar 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Mar 27, 2017

Contributor

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.

Contributor

kevinsawicki commented Mar 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 27, 2017

Contributor

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 😉
Contributor

tarruda commented Mar 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Mar 27, 2017

Contributor

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.

Contributor

kevinsawicki commented Mar 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Mar 27, 2017

Contributor

👍

Maybe leave it for electron 2.0.0 then

Contributor

tarruda commented Mar 27, 2017

👍

Maybe leave it for electron 2.0.0 then

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki May 22, 2017

Contributor

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

Contributor

kevinsawicki commented May 22, 2017

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

@kevinsawicki kevinsawicki deleted the optimize-sendsync branch May 22, 2017

@ckerr ckerr restored the optimize-sendsync branch Feb 6, 2018

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Feb 6, 2018

Member

Reopening for consideration in the upcoming post-2.0 world

Member

ckerr commented Feb 6, 2018

Reopening for consideration in the upcoming post-2.0 world

@ckerr ckerr reopened this Feb 6, 2018

@jkleinsc

This comment has been minimized.

Show comment
Hide comment
@jkleinsc

jkleinsc Feb 6, 2018

Contributor

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

Contributor

jkleinsc commented Feb 6, 2018

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

@tarruda tarruda requested a review from electron/reviewers as a code owner Feb 7, 2018

@zcbenz

zcbenz approved these changes Feb 13, 2018

Looks like a good thing to have.

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Feb 13, 2018

Member

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"

Member

MarshallOfSound commented Feb 13, 2018

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"

@poiru poiru added this to Candidates in 3.0.x Feb 27, 2018

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Mar 14, 2018

Member

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?

Member

ckerr commented Mar 14, 2018

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

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 28, 2018

Contributor

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

Contributor

miniak commented May 28, 2018

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

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 28, 2018

Contributor

@tarruda I'd definitely like to see this merged. I did a simple benchmark:

var electron = require('electron')
var fs = electron.remote.require('fs');
console.time('test'); var data = fs.readFileSync('filename'); console.timeEnd('test');
  • original implementation: 2200 ms
  • after my change (#13055): 350 ms
  • after your change: 150 ms
  • baseline (direct call without remote): 12 ms
Contributor

miniak commented May 28, 2018

@tarruda I'd definitely like to see this merged. I did a simple benchmark:

var electron = require('electron')
var fs = electron.remote.require('fs');
console.time('test'); var data = fs.readFileSync('filename'); console.timeEnd('test');
  • original implementation: 2200 ms
  • after my change (#13055): 350 ms
  • after your change: 150 ms
  • baseline (direct call without remote): 12 ms
@miniak

miniak approved these changes May 29, 2018

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 29, 2018

Contributor

@zcbenz, @MarshallOfSound can you please review the latest version? thanks!

Contributor

miniak commented May 29, 2018

@zcbenz, @MarshallOfSound can you please review the latest version? thanks!

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz May 29, 2018

Contributor

The latest version looks good to me. 👍

Contributor

zcbenz commented May 29, 2018

The latest version looks good to me. 👍

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda May 29, 2018

Contributor

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

Thanks @miniak 👍

Contributor

tarruda commented May 29, 2018

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

Thanks @miniak 👍

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

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 29, 2018

Contributor

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

Contributor

miniak commented May 29, 2018

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

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 30, 2018

Contributor

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

Contributor

miniak commented May 30, 2018

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

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 30, 2018

Contributor

@zcbenz can you please merge?

Contributor

miniak commented May 30, 2018

@zcbenz can you please merge?

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound May 30, 2018

Member

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

Member

MarshallOfSound commented May 30, 2018

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

@MarshallOfSound

Want to talk

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 30, 2018

Contributor

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

Contributor

miniak commented May 30, 2018

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

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 30, 2018

Contributor

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

Contributor

miniak commented May 30, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound May 30, 2018

Member

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

Member

MarshallOfSound commented May 30, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak May 30, 2018

Contributor

@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?
Contributor

miniak commented May 30, 2018

@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?
Don't use JSON to send the result of `ipcRenderer.sendSync`.
- 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

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak Jun 8, 2018

Contributor

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

Contributor

miniak commented Jun 8, 2018

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

@miniak

This comment has been minimized.

Show comment
Hide comment
@miniak

miniak Jun 11, 2018

Contributor

@MarshallOfSound can you unblock the PR?

Contributor

miniak commented Jun 11, 2018

@MarshallOfSound can you unblock the PR?

@MarshallOfSound

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

@MarshallOfSound MarshallOfSound merged commit 6ff111a into master Jun 13, 2018

11 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

3.0.x automation moved this from Candidates to Fixed Jun 13, 2018

@MarshallOfSound MarshallOfSound deleted the optimize-sendsync branch Jun 13, 2018

@ckerr ckerr removed this from Fixed in 3.0.x Jul 12, 2018

@miniak miniak referenced this pull request Aug 5, 2018

Merged

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

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment