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

refactor: use v8 serialization for ipc #20214

Merged
merged 56 commits into from Oct 9, 2019

Conversation

@nornagon
Copy link
Member

commented Sep 12, 2019

Description of Change

BREAKING CHANGE

This changes IPC communication to use v8's Structured Clone algorithm instead of using the base::Value serialization defined in native_mate_converters/v8_value_converter.cc. This is be faster, more featureful, and less surprising than the existing logic, since it more-or-less matches the logic that backs postMessage.

This brings about a 2x performance boost for large buffers and complex objects. Latency for small messages is not significantly affected.

User-observable differences from the existing IPC API:

  • It's about 2x faster.
  • NaN, Infinity, and undefined are transferred as such, rather than being converted to null.
  • Cyclic objects can be transmitted.
  • Set and Map, Error, RegExp, Date and BigInt can be transmitted.
  • Buffer will be converted to Uint8Array.
  • Typed arrays (Float32Array and friends) will be transmitted as they are, instead of being converted to Buffer.
  • Sparse arrays will be transferred as sparse arrays, instead of being converted to dense arrays with nulls.

NOTE: Objects that aren't serializable with V8's Structured Clone algorithm, such as functions, DOM objects, special Node/Electron objects like process.env or WebContents, or any objects containing such items will be serialized with the old base::Value-based algorithm. However, this behavior is deprecated and will throw an exception beginning with Electron 9.

Checklist

Release Notes

Notes: IPC between main and renderer processes now uses the Structured Clone Algorithm.

@nornagon

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

I did some preliminary performance testing. The numbers below are the average time taken, in milliseconds, to send a Uint8Array buffer of a given size, across 200 calls to send().

With the existing mojo-based base::Value serialization:

200 x 2k: 0.42
200 x 4k: 0.60
200 x 8k: 0.93
200 x 16k: 1.61
200 x 32k: 2.88
200 x 64k: 5.50
200 x 128k: 10.74
200 x 256k: 21.07
200 x 512k: 42.62

With this PR, using v8's ValueSerializer:

200 x 2k: 0.44
200 x 4k: 0.55
200 x 8k: 0.76
200 x 16k: 1.08
200 x 32k: 1.90
200 x 64k: 3.44
200 x 128k: 6.53
200 x 256k: 12.59
200 x 512k: 23.99

So this seems to be a little less than twice as fast for this use case.

@nornagon

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

Ref #18758

@jkleinsc

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Could we possibly support both the existing serialization and the sca approach at the same time? I'm wondering if we can follow a deprecation path here or if this will have to be all or nothing.

@nornagon

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

Tried this with some larger buffers:

Before:

10 x 1024k: 93.44
10 x 2048k: 183.62
10 x 4096k: 352.30
10 x 8192k: 727.64
10 x 16384k: 1443.97
10 x 32768k: 2845.11
10 x 65536k: 5565.72
---crashed with buffers of size 128M---

after:

10 x 1024k: 69.51
10 x 2048k: 100.01
10 x 4096k: 194.63
10 x 8192k: 386.91
10 x 16384k: 774.37
10 x 32768k: 1606.11
10 x 65536k: 3262.18
10 x 131072k: 6703.16
10 x 262144k: 13136.74
@nornagon

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

@jkleinsc I'd like to first try to find a way we can make this at least more compatible with the existing structure, if not 1:1. If we can find a way that works for 99% of use cases, then I think we should cut over directly and accept the breakage. If we find fundamental issues that are not paper-over-able, then we should investigate ways we can expose this functionality differently (perhaps a new set of APIs like ipcRenderer.post(), or perhaps a deprecation cycle of some sort).

@nornagon

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

I also tried this with complex objects. The numbers below are for NxM objects, where N is the breadth and M is the depth. For example, a 2x3 object is:

{'key-0': {'key-0': {'key-0': {'key-0': {}, 'key-1': {}}, 'key-1': {'key-0': {}, 'key-1': {}}}, 'key-1': {'key-0': {'key-0': {}, 'key-1': {}}, 'key-1': {'key-0': {}, 'key-1': {}}}}, 'key-1': {'key-0': {'key-0': {'key-0': {}, 'key-1': {}}, 'key-1': {'key-0': {}, 'key-1': {}}}, 'key-1': {'key-0': {'key-0': {}, 'key-1': {}}, 'key-1': {'key-0': {}, 'key-1': {}}}}}

A NaN represents a renderer crash.

Before:

10 x 1x1: 3.34
10 x 1x2: 3.34
10 x 1x4: 3.26
10 x 1x8: 3.30
10 x 1x16: 3.35
10 x 2x1: 3.48
10 x 2x2: 3.42
10 x 2x4: 3.77
10 x 2x8: 10.01
10 x 2x16: 1736.84
10 x 4x1: 3.21
10 x 4x2: 3.41
10 x 4x4: 7.92
10 x 4x8: 1142.82
10 x 4x16: NaN
10 x 8x1: 3.52
10 x 8x2: 4.08
10 x 8x4: 64.33
10 x 8x8: NaN
10 x 8x16: NaN
10 x 16x1: 4.52
10 x 16x2: 7.23
10 x 16x4: 1003.91
10 x 16x8: NaN
10 x 16x16: NaN

After:

10 x 1x1: 3.38
10 x 1x2: 4.00
10 x 1x4: 3.37
10 x 1x8: 3.01
10 x 1x16: 3.06
10 x 2x1: 4.38
10 x 2x2: 4.14
10 x 2x4: 3.67
10 x 2x8: 5.41
10 x 2x16: 409.08
10 x 4x1: 3.07
10 x 4x2: 3.65
10 x 4x4: 3.85
10 x 4x8: 234.70
10 x 4x16: NaN
10 x 8x1: 3.51
10 x 8x2: 3.29
10 x 8x4: 15.58
10 x 8x8: 87138.07
10 x 8x16: NaN
10 x 16x1: 3.93
10 x 16x2: 4.05
10 x 16x4: 181.39
10 x 16x8: NaN
10 x 16x16: NaN

So, quite a bit faster. I think the crashes are when constructing those stupidly large objects, rather than serializing them. And the new code does succeed on the 8x8 case, where the old code crashed.

@electron-cation electron-cation bot removed the new-pr 🌱 label Sep 13, 2019
@nornagon nornagon force-pushed the cloneable-message-ipc branch from 93ab58d to 8301091 Sep 16, 2019
@nornagon nornagon requested a review from electron/wg-upgrades as a code owner Sep 16, 2019
@nornagon nornagon referenced this pull request Sep 19, 2019
5 of 6 tasks complete
Copy link
Contributor

left a comment

can you please address these small concerns? otherwise huge 👍for the great job!

lib/browser/chrome-extension.js Outdated Show resolved Hide resolved
lib/browser/guest-view-manager.js Show resolved Hide resolved
shell/common/api/api.mojom Show resolved Hide resolved
shell/common/api/api.mojom Show resolved Hide resolved
@electron electron deleted a comment from nornagon Oct 4, 2019
@electron electron deleted a comment from nornagon Oct 4, 2019
@electron electron deleted a comment from nornagon Oct 4, 2019
@@ -1068,7 +1068,7 @@ describe('<webview> tag', function () {
await loadWebView(webview, { src })

const data = await webview.printToPDF({})
expect(data).to.be.an.instanceof(Buffer).that.is.not.empty()
expect(data).to.be.an.instanceof(Uint8Array).that.is.not.empty()

This comment has been minimized.

Copy link
@miniak

miniak Oct 4, 2019

Contributor

the documentation for <webview>.printToPDF(options) should specify the return value as Promise<Uint8Array> instead of Promise<Buffer>

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 4, 2019

Author Member

Unfortunately this currently produces an error in our typings generator:

../electron.d.ts(10212,62): error TS2694: Namespace 'Electron' has no exported member 'Uint8Array'.

This comment has been minimized.

Copy link
@nornagon

This comment has been minimized.

Copy link
@nornagon
docs/api/breaking-changes.md Show resolved Hide resolved
docs/api/breaking-changes.md Outdated Show resolved Hide resolved
@miniak

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2019

this will allow us to simplify the serialization in the remote module. #20427

nornagon added 4 commits Oct 4, 2019
@miniak
miniak approved these changes Oct 4, 2019
Copy link
Member

left a comment

Very cool!

Comments / questions inline, but nothing fundamental. Overall this is really nice.

docs/api/breaking-changes.md Show resolved Hide resolved
docs/api/ipc-renderer.md Show resolved Hide resolved
lib/browser/rpc-server.js Show resolved Hide resolved
shell/browser/api/atom_api_web_contents.cc Outdated Show resolved Hide resolved
spec-main/api-ipc-renderer-spec.ts Show resolved Hide resolved
@ckerr
ckerr approved these changes Oct 9, 2019
Copy link
Member

left a comment

Seems good

@jkleinsc jkleinsc merged commit 2fad53e into master Oct 9, 2019
14 of 15 checks passed
14 of 15 checks passed
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20191008.17 succeeded
Details
electron-arm64-testing Build #20191008.17 succeeded
Details
electron-woa-testing Build #20191008.14 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Oct 9, 2019

Release Notes Persisted

IPC between main and renderer processes now uses the Structured Clone Algorithm.

@miniak miniak deleted the cloneable-message-ipc branch Oct 9, 2019
@nornagon nornagon referenced this pull request Oct 9, 2019
5 of 5 tasks complete
@Xylobol

This comment has been minimized.

Copy link

commented Oct 10, 2019

For anyone else interested, these changes have landed in nightly - https://github.com/electron/nightlies/releases/tag/v8.0.0-nightly.20191009

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