Skip to content
This repository has been archived by the owner on May 25, 2021. It is now read-only.

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

Merged
merged 5 commits into from
Sep 6, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 137 additions & 52 deletions src/backend/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
MutableTree,
Node,
Path,
deserializePath,
} from '../tree';

import {createTreeFromElements} from '../tree/mutable-tree-factory';
Expand All @@ -15,39 +14,67 @@ import {
Message,
MessageFactory,
MessageType,
browserDispatch,
browserSubscribe,
} from '../communication';

import {send} from './indirect-connection';

import {
Route,
MainRoute,
defineLookupOperation,
highlight,
parseRoutes,
} from './utils';

import {serialize} from '../utils';

import {MessageQueue} from '../structures';

import {SimpleOptions} from '../options';

declare const ng;
declare const getAllAngularRootElements: () => Element[];
declare const treeRenderOptions: SimpleOptions;

/// For tree deltas that contain more changes than {@link deltaThreshold},
/// 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;
Copy link
Contributor

@xorgy xorgy Sep 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// For large messages, we do not send them through the normal pipe (which
/// is backend > content script > backround channel > frontend), we add them
/// to this buffer and then send a {@link MessageType.Push} message that
/// tells the frontend to read messages directly from this queue itself.
/// This allows us to prevent very large messages containing tree data from
/// being serialized and deserialized four times. Using this mechanism, they
/// are serialized and deserialized a total of one times.
const messageBuffer = new MessageQueue<Message<any>>();

/// NOTE(cbond): We collect roots from all applications (mulit-app support)
let previousTree: MutableTree;

let previousCount: number;

const updateTree = (roots: Array<DebugElement>) => {
const showElements = treeRenderOptions.showElements;

const newTree = createTreeFromElements(roots, showElements);
const {tree, count} = createTreeFromElements(roots, showElements);

if (previousTree == null || Math.abs(previousCount - count) > deltaThreshold) {
messageBuffer.enqueue(MessageFactory.completeTree(tree));
}
else {
messageBuffer.enqueue(MessageFactory.treeDiff(previousTree.diff(tree)));
}

/// Send a message through the normal channels to indicate to the frontend
/// that messages are waiting for it in {@link messageBuffer}
send<void, void>(MessageFactory.push());

send<void, any>(
previousTree
? MessageFactory.treeDiff(previousTree.diff(newTree))
: MessageFactory.completeTree(newTree));
previousTree = tree;
Copy link

@igor-ka igor-ka Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


previousTree = newTree;
previousCount = count;
};

const update = () => {
Expand All @@ -69,60 +96,63 @@ const bind = (root: DebugElement) => {

getAllAngularRootElements().forEach(root => bind(ng.probe(root)));

browserSubscribe(
(message: Message<any>) => {
switch (message.messageType) {
case MessageType.Initialize:
// Update our tree settings closure
Object.assign(treeRenderOptions, message.content);
const messageHandler = (message: Message<any>) => {
switch (message.messageType) {
case MessageType.Initialize:
// Update our tree settings closure
Object.assign(treeRenderOptions, message.content);

// Clear out existing tree representation and start over
previousTree = null;
// Clear out existing tree representation and start over
previousTree = null;

// Load the complete component tree
subject.next(void 0);
// Load the complete component tree
subject.next(void 0);

return true;
return true;

case MessageType.SelectComponent:
return tryWrap(() => {
const path: Path = message.content.path;
case MessageType.SelectComponent:
return tryWrap(() => {
const path: Path = message.content.path;

const node = previousTree.traverse(path);
const node = previousTree.traverse(path);

this.consoleReference(node);
this.consoleReference(node);

// For component selection events, we respond with component instance
// properties for the selected node. If we had to serialize the
// properties of each node on the tree that would be a performance
// killer, so we only send the componentInstance values for the
// node that has been selected.
if (message.content.requestInstance) {
return getComponentInstance(previousTree, node);
}
});
// For component selection events, we respond with component instance
// properties for the selected node. If we had to serialize the
// properties of each node on the tree that would be a performance
// killer, so we only send the componentInstance values for the
// node that has been selected.
if (message.content.requestInstance) {
return getComponentInstance(previousTree, node);
}
});

case MessageType.UpdateProperty:
return tryWrap(() => updateProperty(previousTree,
message.content.path,
message.content.newValue));
case MessageType.UpdateProperty:
return tryWrap(() => updateProperty(previousTree,
message.content.path,
message.content.newValue));

case MessageType.EmitValue:
return tryWrap(() => emitValue(previousTree,
message.content.path,
message.content.value));
case MessageType.EmitValue:
return tryWrap(() => emitValue(previousTree,
message.content.path,
message.content.value));

case MessageType.RouterTree:
return tryWrap(() => routerTree());
case MessageType.RouterTree:
return tryWrap(() => routerTree());

case MessageType.Highlight:
const nodes = message.content.nodes
.map(id => previousTree.search(id));
case MessageType.Highlight:
if (previousTree == null) {
return;
}
return tryWrap(() => {
highlight(message.content.nodes.map(id => previousTree.lookup(id)));
});
}
return undefined;
};

return tryWrap(() => highlight(nodes));
}
return undefined;
});
browserSubscribe(messageHandler);

// We do not store component instance properties on the node itself because
// we do not want to have to serialize them across backend-frontend boundaries.
Expand Down Expand Up @@ -226,4 +256,59 @@ export const tryWrap = (fn: Function) => {
}
};

defineLookupOperation(() => previousTree);
/// We need to define some operations that are accessible from the global scope so that
/// the frontend can invoke them using {@link inspectedWindow.eval}. But we try to do it
/// in a safe way and ensure that we do not overwrite any existing properties or functions
/// that share the same names. If we do encounter such things we throw an exception and
/// complain about it instead of continuing with bootstrapping.
export const defineWindowOperations = <T>(target, classImpl: T) => {
for (const key of Object.keys(classImpl)) {
if (target[key] != null) {
throw new Error(`A window function or object named ${key} would be overwritten`);
}
}

Object.assign(target, classImpl);
};

export class WindowOperations {
/// Note that the ID is a serialized path, and the first element in that path is the
/// index of the application that the node belongs to. So even though we have this
/// global lookup operation for things like 'inspect' and 'view source', it will find
/// the correct node even if multiple applications are instantiated on the same page.
nodeFromPath(id: string): Element {
if (previousTree == null) {
throw new Error('No tree exists');
}

const node = previousTree.lookup(id);
if (node == null) {
console.error(`Cannot find element associated with node ${id}`);
return null;
}
return node.nativeElement();
}

/// Post a response to a message from the frontend and dispatch it through normal channels
response<T>(response: Message<T>) {
browserDispatch(response);
}

/// Run the message handler and return the result immediately instead of posting a response
handleImmediate<T>(message: Message<T>) {
const result = messageHandler(message);
if (result) {
return serialize(result);
}
return null;
}

/// Read all messages in the buffer and remove them
readMessageQueue(): Array<Message<any>> {
return messageBuffer.dequeue();
}
}

const windowOperationsImpl = new WindowOperations();

defineWindowOperations(window || global || this, {inspectedApplication: windowOperationsImpl});
18 changes: 18 additions & 0 deletions src/backend/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,24 @@ export const subscribe = (handler: MessageHandler) => {
};

export const send = <T>(message: Message<T>) => {
switch (message.messageType) {
/// These types of messages should never be sent through this mechanism. A DispatchWrapper
/// message is for communication between content-script and the backend and has no business
/// being sent to the frontend. Similarly, a message containing tree data should be sent
/// through the {@link MessageBuffer} mechanism in backend.ts instead of through this port.
/// Sending a message with the {@link send} function will cause that message to take a very
/// 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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Replaced with if statement

case MessageType.TreeDiff:
case MessageType.DispatchWrapper:
const description = MessageType[message.messageType];
throw new Error(`A ${description} message should never be posted through the communication port`);
default:
break;
}

return new Promise((resolve, reject) => {
chrome.runtime.sendMessage(message,
response => {
Expand Down
4 changes: 2 additions & 2 deletions src/backend/indirect-connection.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import {
Message,
MessageFactory,
browserDispatch,
messageJumpContext,
browserSubscribeResponse,
} from '../communication';

export const send = <Response, T>(message: Message<T>): Promise<Response> => {
return new Promise((resolve, reject) => {
browserSubscribeResponse(message.messageId, response => resolve(response));
browserDispatch(MessageFactory.dispatchWrapper(message));
messageJumpContext(MessageFactory.dispatchWrapper(message));
});
};

1 change: 0 additions & 1 deletion src/backend/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export * from './description';
export * from './highlighter';
export * from './lookup';
export * from './parse-router';
27 changes: 0 additions & 27 deletions src/backend/utils/lookup.ts

This file was deleted.

Loading