Major performance improvment by bypassing the sequence of ports when sending tree data #592

clbond · 2016-09-02T17:42:10Z

This PR represents a major performance improvement over previous builds.

Before this change, we were sending tree data through the normal channel. That meant every time we sent a tree or a tree delta, it had to go through this sequence:

Backend script -> content script -> background channel -> frontend

Each one of these steps required a serialize + deserialize (which happens in the internal Chrome code, not our code). Therefore for large trees, it was an absolute performance killer. This PR fixes that. Now the trees are just serialized and deserialized once on their way from the backend to the frontend.

I encourage anyone who reviews this to do a side-by-side performance comparison using the Kitchen Sink stress tester or any other app.

* Revert the recursion change I made to the serializer as it performs way worse than the old implementation * Do not expand large arrays by default in the State view because it takes a long time to render them (Note: There is a big performance fix coming around message transmission but this is not it)

alternate channel instead of the sequence of ports that require 4 serializations and deserializations for each message

mention-bot · 2016-09-02T17:42:11Z

@clbond, thanks for your PR! By analyzing the annotation information on this pull request, we identified @xorgy, @sumitarora and @ericjim to be potential reviewers

xorgy · 2016-09-02T19:32:02Z

src/backend/backend.ts

+/// we simply send the entire tree again instead of trying to patch it
+/// since it will be faster than trying to apply hundreds or thousands of
+/// changes to an existing tree.
+const deltaThreshold = 512;


Did you profile to find this parameter, or is it very ballpark?

I don't have measurements. But I did do a bit of testing around it and it appeared to improve performance. Logically it also makes sense. There is no sense trying to compare two massive trees that you already know are vastly different. Just send the new one. Other than that does the PR look OK? Did you try running it?

I am ok with 512 being a ballpark estimate at this point

oskarols · 2016-09-06T09:15:45Z

Tried testing it on a large RC4 app, component tree is showing up blank unfortunately.
No errors in the background page. Lots of "got msg from backend" messages in the console.

I'm assuming this only works for RC5 or 6?

clbond · 2016-09-06T11:52:37Z

@oskarols Are there any exceptions in the backend or the frontend? For backend, you can just open the Console tab that you would normally use for your app. For the frontend, open Augury inside a popup (not docked) and then hit Cmd-Shift-I (Mac) or Ctrl-Shift-I (Windows).

Is your app publicly available so I can reproduce?

oskarols · 2016-09-06T14:19:44Z

Sadly it's not publicly available. Seeing no exceptions in either.

clbond · 2016-09-06T14:56:30Z

If you can make a plunkr that reproduces the issue I would love to debug and fix it. I have used Augury for RC4 appals and it appeared to work properly. Alternatively if you have a bit of time to do a screen-sharing session with me, we could look into the issue together.

igor-ka · 2016-09-06T18:47:01Z

src/backend/connection.ts

+    /// circuitous route and will be serialized and deserialized repeatedly. Therefore large
+    /// messages must be sent using the {@link MessageBuffer} mechanism in order to avoid major
+    /// performance bottlenecks and UI latency.
+    case MessageType.CompleteTree:


@clbond Do we need those 2 empty case conditions here?

Yes, they fall through to the error handler. The alternative would be to do:

if (messageType == MessageType.TreeDiff || messageType == MessageType.CompleteTree || messageType == MessageType.DispatchWrapper) { throw ...; }

Fixed. Replaced with if statement

igor-ka · 2016-09-06T19:11:58Z

src/backend/backend.ts

-    previousTree
-      ? MessageFactory.treeDiff(previousTree.diff(newTree))
-      : MessageFactory.completeTree(newTree));
+  previousTree = tree;


In the future let's extract the variable assignment out of it. One way is to have a redux store on the backend that will manage this. Or simply wrap this in a class. This is completely out of scope though.

Alright, I will make a note of this to address in the near future.

igor-ka · 2016-09-06T19:25:52Z

@clbond LGTM

clbond · 2016-09-06T19:26:29Z

Thank you Igor. I appreciate your thorough review :)

clbond · 2016-09-07T15:41:53Z

@oskarols Are you trying to debug an app that is running in production mode?

oskarols · 2016-09-09T07:33:54Z

@clbond Nope, not in production mode. Looked into it, and it seems we're getting some exception from Augury.

zone.js:269 Uncaught TypeError: Cannot read property 'name' of undefined

exports.functionName                @   function-name.ts?6954:7
(anonymous function)                @   transformer.ts?d0de:89
dependencies                        @   transformer.ts?d0de:89
(anonymous function)                @   transformer.ts?d0de:137
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
(anonymous function)                @   transformer.ts?d0de:153
transformChildren                   @   transformer.ts?d0de:151
(anonymous function)                @   transformer.ts?d0de:172
load                                @   transformer.ts?d0de:53
exports.transform                   @   transformer.ts?d0de:61
exports.transformToTree             @   mutable-tree-factory.ts?f7a1:12
(anonymous function)                @   mutable-tree-factory.ts?f7a1:40
exports.createTreeFromElements      @   mutable-tree-factory.ts?f7a1:40
updateTree                      @   backend.ts:64
update                          @   backend.ts:83
(anonymous function)                @   backend.ts:94

(stack trace continues, but those are all the parts related to Augury as far as I can tell)

Ping me if you want to do some screen-sharing debugging.

tb · 2016-09-09T10:48:52Z

I have same error on Angular2 2.0.0-rc.5 in development

Uncaught TypeError: Cannot read property 'name' of undefined
exports.functionName    @   function-name.ts?6954:7

clbond added 3 commits September 2, 2016 09:03

Major performance improvement by sending component data through an

206f64e

alternate channel instead of the sequence of ports that require 4 serializations and deserializations for each message

Up the delta threshold

081f070

clbond added the pr: review-needed label Sep 2, 2016

xorgy reviewed Sep 2, 2016
View reviewed changes

Remove debug statements

5374ba7

igor-ka reviewed Sep 6, 2016
View reviewed changes

Remove fallthrough switch statement (Igor's comment)

59658e0

igor-ka reviewed Sep 6, 2016
View reviewed changes

clbond merged commit db9310a into rangle:dev Sep 6, 2016

clbond removed the pr: review-needed label Sep 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major performance improvment by bypassing the sequence of ports when sending tree data #592

Major performance improvment by bypassing the sequence of ports when sending tree data #592

clbond commented Sep 2, 2016

mention-bot commented Sep 2, 2016

xorgy Sep 2, 2016 •

edited

Loading

clbond Sep 2, 2016

igor-ka Sep 6, 2016

oskarols commented Sep 6, 2016 •

edited

Loading

clbond commented Sep 6, 2016

oskarols commented Sep 6, 2016

clbond commented Sep 6, 2016

igor-ka Sep 6, 2016

clbond Sep 6, 2016

clbond Sep 6, 2016

igor-ka Sep 6, 2016 •

edited

Loading

clbond Sep 6, 2016

igor-ka commented Sep 6, 2016

clbond commented Sep 6, 2016

clbond commented Sep 7, 2016

oskarols commented Sep 9, 2016 •

edited

Loading

tb commented Sep 9, 2016 •

edited

Loading

Major performance improvment by bypassing the sequence of ports when sending tree data #592

Major performance improvment by bypassing the sequence of ports when sending tree data #592

Conversation

clbond commented Sep 2, 2016

mention-bot commented Sep 2, 2016

xorgy Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

clbond Sep 2, 2016

Choose a reason for hiding this comment

igor-ka Sep 6, 2016

Choose a reason for hiding this comment

oskarols commented Sep 6, 2016 • edited Loading

clbond commented Sep 6, 2016

oskarols commented Sep 6, 2016

clbond commented Sep 6, 2016

igor-ka Sep 6, 2016

Choose a reason for hiding this comment

clbond Sep 6, 2016

Choose a reason for hiding this comment

clbond Sep 6, 2016

Choose a reason for hiding this comment

igor-ka Sep 6, 2016 • edited Loading

Choose a reason for hiding this comment

clbond Sep 6, 2016

Choose a reason for hiding this comment

igor-ka commented Sep 6, 2016

clbond commented Sep 6, 2016

clbond commented Sep 7, 2016

oskarols commented Sep 9, 2016 • edited Loading

tb commented Sep 9, 2016 • edited Loading

xorgy Sep 2, 2016 •

edited

Loading

oskarols commented Sep 6, 2016 •

edited

Loading

igor-ka Sep 6, 2016 •

edited

Loading

oskarols commented Sep 9, 2016 •

edited

Loading

tb commented Sep 9, 2016 •

edited

Loading