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

Fix optimizeSimpleObject for arrays of objects #12815

Merged
merged 1 commit into from May 4, 2018

Conversation

Projects
None yet
5 participants
@miniak
Contributor

miniak commented May 3, 2018

Before the fix, calling an API via remote, which returns an array of simple objects, such as electron.screen.getAllDisplays() would return an array of object proxies, where accessing every single property would cause an synchronous IPC.

@miniak miniak requested a review from electron/reviewers as a code owner May 3, 2018

@miniak miniak requested review from alexeykuzmin, zcbenz and alespergl May 3, 2018

@miniak

This comment has been minimized.

Contributor

miniak commented May 3, 2018

We should consider backporting this to 1.8 and 2.0

@ckerr

This comment has been minimized.

Member

ckerr commented May 3, 2018

The diff itself looks straightforward.

Are we changing any implicit contracts here? E.g. if these objects were proxied before, could objects have been used as a way of passing data back and forth via object properties?

If so I wouldn't see this fix as nonbreaking & would limit to master and not backport

@miniak

This comment has been minimized.

Contributor

miniak commented May 3, 2018

@ckerr Objects, which were supposed to be passed as a value were proxied when they were placed in an array. This does not make sense to me. I need to check git history to see who implemented it and ask whether this was a deliberate decision or just an oversight leading to a bug.

@miniak

This comment has been minimized.

Contributor

miniak commented May 3, 2018

before the fix
screen shot 2018-05-03 at 7 50 39 pm

after the fix
screen shot 2018-05-03 at 7 49 45 pm

@miniak

This comment has been minimized.

Contributor

miniak commented May 3, 2018

Also when a property value is being retrieved, we're not calling valueToMeta with optimizeSimpleObject == true, which does not make much sense either, as a property getter is basically a function (optimizeSimpleObject is only applied to function call return values)

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 3, 2018

I need to check git history to see who implemented it and ask whether this was a deliberate decision or just an oversight leading to a bug.

That's a good idea.

@miniak

This comment has been minimized.

Contributor

miniak commented May 3, 2018

This is the original commit: f7c75d3

@miniak

This comment has been minimized.

Contributor

miniak commented May 3, 2018

@zcbenz do you remember why you didn't check for the hidden simple property unconditionally when dealing with any object when making this change? Performance concerns?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 3, 2018

Are we changing any implicit contracts here? E.g. if these objects were proxied before, could objects have been used as a way of passing data back and forth via object properties?

@ckerr
Implicit contracts are not contracts =)

@miniak

This comment has been minimized.

Contributor

miniak commented May 3, 2018

Thinking about it more, it feels like it would make sense to mark arrays simple as well in these places:

  • V8ValueConverter::ToV8Array
  • mate::Converter<std::vector<T>>

That would make serializing them much faster. This would be the change in valueToMeta:

diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js
index 0caac2293..ff4b3f487 100644
--- a/lib/browser/rpc-server.js
+++ b/lib/browser/rpc-server.js
@@ -55,12 +55,13 @@ let getObjectPrototype = function (object) {
 }
 
 // Convert a real value into meta data.
-let valueToMeta = function (sender, value, optimizeSimpleObject = false) {
+let valueToMeta = function (sender, value) {
   // Determine the type of value.
   const meta = { type: typeof value }
   if (meta.type === 'object') {
     // Recognize certain types of objects.
-    if (value === null) {
+    if (value === null || v8Util.getHiddenValue(value, 'simple')) {
+      // Treat simple objects as value.
       meta.type = 'value'
     } else if (ArrayBuffer.isView(value)) {
       meta.type = 'buffer'
@@ -75,15 +76,12 @@ let valueToMeta = function (sender, value, optimizeSimpleObject = false) {
     } else if (hasProp.call(value, 'callee') && value.length != null) {
       // Treat the arguments object as array.
       meta.type = 'array'
-    } else if (optimizeSimpleObject && v8Util.getHiddenValue(value, 'simple')) {
-      // Treat simple objects as value.
-      meta.type = 'value'
     }
   }
 
   // Fill the meta object according to value's type.
   if (meta.type === 'array') {
-    meta.members = value.map((el) => valueToMeta(sender, el, optimizeSimpleObject))
+    meta.members = value.map((el) => valueToMeta(sender, el))
   } else if (meta.type === 'object' || meta.type === 'function') {
     meta.name = value.constructor ? value.constructor.name : ''
 
@@ -242,12 +240,12 @@ const callFunction = function (event, func, caller, args) {
   try {
     if (funcMarkedAsync && !funcPassedCallback) {
       args.push(function (ret) {
-        event.returnValue = valueToMeta(event.sender, ret, true)
+        event.returnValue = valueToMeta(event.sender, ret)
       })
       func.apply(caller, args)
     } else {
       ret = func.apply(caller, args)
-      event.returnValue = valueToMeta(event.sender, ret, true)
+      event.returnValue = valueToMeta(event.sender, ret)
     }
   } catch (error) {
     // Catch functions thrown further down in function invocation and wrap
@ckerr

This comment has been minimized.

Member

ckerr commented May 3, 2018

@alexeykuzmin patch and minor versions by definition only get backwards-compatible changes. This isn't backwards-compatible so I'm not sure what else there is to say about this in 2.x.y.

If the proxying is unintended -- and it smells like that to me -- let's document the change in the breaking-changes md and land it for 3.0.0-beta.1

@miniak

This comment has been minimized.

Contributor

miniak commented May 3, 2018

@ckerr I've removed the backport labels. Let's focus now on coming up with the most efficient way of proxying the values. Can you have a look at my proposal above? I will probably create a separate PR for that.

@zcbenz

zcbenz approved these changes May 4, 2018

I can't think of a reason not to do this, I probably just forgot to add this in my original commit.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented May 4, 2018

it feels like it would make sense to mark arrays simple as well in these places:

V8ValueConverter::ToV8Array
mate::Converter<std::vector>

Some arrays do include non-simple objects, for example BrowserWindow.getAll returns an array of windows.

That would make serializing them much faster. This would be the change in valueToMeta:
} else if (optimizeSimpleObject && v8Util.getHiddenValue(value, 'simple')) {
// Treat simple objects as value.
meta.type = 'value'

Currently we only optimize return values from builtin APIs, since it is guaranteed that we won't read them back in the main process. The proposed change also optimizes the objects passed to callbacks, which can be a problem since modifications to those objects will not reflect back to the main process.

@ckerr ckerr merged commit 9b56ca3 into master May 4, 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

@ckerr ckerr deleted the fix-optimizeSimpleObject branch May 4, 2018

@ckerr ckerr added the semver/major label May 4, 2018

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 4, 2018

@alexeykuzmin patch and minor versions by definition only get backwards-compatible changes. This isn't backwards-compatible so I'm not sure what else there is to say about this in 2.x.y.

If the proxying is unintended -- and it smells like that to me -- let's document the change in the breaking-changes md and land it for 3.0.0-beta.1

@ckerr anything that is not documented – implementation details, private APIs, certain behaviours – is not part of a public contract, and can be changed or even removed any time.

If this proxying was not documented anywhere, and we see a benefit in removing it (in decreased number of ICP messages for example) then technically we can backport it to any version.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 4, 2018

If this proxying was not documented anywhere, and we see a benefit in removing it (in decreased number of ICP messages for example) then technically we can backport it to any version.

Although the behavior was not documented / strictly API, people used the API that exhibited this behavior. Changing the behavior, while although does not break an API contract, does change how that contract is implemented in a way that isn't transparent to the user. I.e. This could have performance implications for some users

I agree with @ckerr here, we shouldn't backport this 👍

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