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

Optimize flush #26

Merged
merged 7 commits into from Mar 25, 2016

Conversation

Projects
None yet
2 participants
@burrows
Copy link
Member

burrows commented Mar 23, 2016

When dealing with a large number of property changes (via a load of a lot of data for example) flushing changes to dependent properties and proxy objects was pretty inefficient. This PR aims to optimize that code.

The following changes were made:

  1. Avoid processing the same object/name pair more than once (this easily let to the largest speedup).
  2. Converted the function that processes dependencies to be iterative instead of recursive, thereby eliminating a large number of function calls.
  3. Inlined the function to clear cached values, further reducing the number of function calls.
@burrows

This comment has been minimized.

Copy link
Member

burrows commented Mar 23, 2016

@peterwmwong @alexraginskiy @stallings Mind taking a look at this?

}

while (head) {
let object = head.object;

This comment has been minimized.

@peterwmwong

peterwmwong Mar 24, 2016

Contributor

Maybe to DRY-up the code below and reduce property lookups (ex. object.objectId), we could destructure upfront.

let {name, object, object:{objectId, __deps__, __proxies__}} = head
for (let i = 0, n = deps.length; i < n; i++) {
didChange(object, deps[i]);
if (!seen[object.objectId + deps[i]]) {

This comment has been minimized.

@peterwmwong

peterwmwong Mar 24, 2016

Contributor

Could we store the "seen key" in a variable? Also for the proxyObject.objectId + proxyName below too.
I worry the repetition will be error prone to maintain (update one but forget the other) and probably a slight perf hit (additional lookups on the VM and additional string concatenations)

@burrows

This comment has been minimized.

Copy link
Member

burrows commented Mar 25, 2016

@peterwmwong I implemented your suggestions, thanks!

@burrows burrows merged commit fdb74d1 into master Mar 25, 2016

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