Repository: vigetlabs/microcosm
Pull request #377 (2017-07-26 14:47:18)

Cache domain handlers when possible
This commit changes the way we store snapshots such that they have context into their last operation. This lets us memoize domain handlers. Basically:

1. Is the prior snapshot value the same?
2. Is the prior action status the same?
3. Is the prior action payload the same?

If so, don't invoke the domain handler again!

Testing this in the wild, this has cut extra domain handler invocation for lots of async tasks by about 30%. Pretty neat.

I also believe that we could use this new approach for reporting in the developer tools. We _could_ eventually store each of these snapshots as a diff.

Additionally, if we were to support sub-domains #184, this caching would become more and more powerful as your key paths became more specific. Breaking up complicated state management would get faster.

codecov-io: # [Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=h1) Report
> Merging [#377](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=desc) into [master](https://codecov.io/gh/vigetlabs/microcosm/commit/59e04efde1dec5009c6a87b5c137c6537bf7ed0b?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/vigetlabs/microcosm/pull/377/graphs/tree.svg?token=AaE0FLz9sU&width=650&src=pr&height=150)](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
+ Coverage   99.88%   99.88%   +<.01%     
==========================================
  Files          25       25              
  Lines         861      872      +11     
==========================================
+ Hits          860      871      +11     
  Misses          1        1
```


| [Impacted Files](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/microcosm.js](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=tree#diff-c3JjL21pY3JvY29zbS5qcw==) | `100% <100%> (ø)` | :arrow_up: |
| [src/domain-engine.js](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=tree#diff-c3JjL2RvbWFpbi1lbmdpbmUuanM=) | `100% <100%> (ø)` | :arrow_up: |
| [src/history.js](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=tree#diff-c3JjL2hpc3RvcnkuanM=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=footer). Last update [59e04ef...e0bd15d](https://codecov.io/gh/vigetlabs/microcosm/pull/377?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

efatsi: really cool stuff, the tests are a bit dense and tough to follow since you're using the same function to define an action as well as what the domain calls in response to the action, but overall 👍 
nhunzaker: @efatsi I've made a new `rebase` method that performs the operations you mentioned regarding `recall`. I've also taken a second pass to improve the clarity of those tests.

Patch:
diff --git a/flow/snapshot.js b/flow/snapshot.js
new file mode 100644
index 00000000..cb6fc82b
--- /dev/null
+++ b/flow/snapshot.js
@@ -0,0 +1,20 @@
+/**
+ * @fileoverview A snapshot is a reference to a specific dispatch for
+ * a given action. A microcosm instance maintains a bank of them to
+ * efficiently update state.
+ * @flow
+ */
+
+type State = { [key: string]: * }
+
+declare type Snapshot = {
+  // The last state the snapshot was dispatched with. This is used
+  // to memoize domain handlers when possible.
+  last: State,
+  // The next state for the snapshot. The outcome of `updateSnapshot`.
+  next: State,
+  // Last known status of the action associated with this snapshot.
+  status: Status,
+  // Last known payload of the action associated with this snapshot.
+  payload: *
+}
diff --git a/src/domain-engine.js b/src/domain-engine.js
index 096f9a45..6ad0e6c8 100644
--- a/src/domain-engine.js
+++ b/src/domain-engine.js
@@ -85,17 +85,30 @@ class DomainEngine {
     return domain
   }
 
-  dispatch(state: Object, action: Action): Object {
+  dispatch(action: Action, state: Object, snapshot: Snapshot) {
     let handlers = this.register(action)
     let result = state
 
     for (var i = 0, len = handlers.length; i < len; i++) {
       var { key, source, handler } = handlers[i]
 
-      var last = get(result, key)
-      var next = handler.call(source, last, action.payload)
-
-      result = set(result, key, next)
+      var base = get(result, key)
+      var head = get(snapshot.last, key)
+
+      if (
+        // If the reference to the prior state changed
+        base !== head ||
+        // Or the payload is different
+        action.payload !== snapshot.payload ||
+        // or the status is different
+        action.status !== snapshot.status
+      ) {
+        // Yes: recalculate state from the base
+        result = set(result, key, handler.call(source, base, action.payload))
+      } else {
+        // No: use the existing snapshot value (memoizing the domain handler)
+        result = set(result, key, get(snapshot.next, key))
+      }
     }
 
     return result
diff --git a/src/history.js b/src/history.js
index 77b2f80c..e86b4b26 100644
--- a/src/history.js
+++ b/src/history.js
@@ -278,15 +278,16 @@ class History extends Emitter {
     console.assert(action, 'History should never reconcile ' + typeof action)
 
     let focus = action
-
     while (focus) {
-      this._emit('update', focus)
+      if (focus.command !== START) {
+        this._emit('update', focus)
+      }
 
       if (focus === this.head) {
         break
-      } else {
-        focus = focus.next
       }
+
+      focus = focus.next
     }
 
     this.archive()
diff --git a/src/microcosm.js b/src/microcosm.js
index ca249ca0..f5b3f554 100644
--- a/src/microcosm.js
+++ b/src/microcosm.js
@@ -95,7 +95,7 @@ class Microcosm extends Emitter implements Domain {
   initial: Object
   state: Object
   history: History
-  snapshots: { [key: string]: * }
+  snapshots: { [key: string]: Snapshot }
   domains: DomainEngine
   effects: EffectEngine
   changes: CompareTree
@@ -112,7 +112,7 @@ class Microcosm extends Emitter implements Domain {
 
     this.history = this.parent ? this.parent.history : new History(options)
 
-    this.snapshots = {}
+    this.snapshots = Object.create(this.parent ? this.parent.snapshots : null)
     this.domains = new DomainEngine(this)
     this.effects = new EffectEngine(this)
     this.changes = new CompareTree(this.state)
@@ -193,37 +193,65 @@ class Microcosm extends Emitter implements Domain {
     return this.initial == null ? {} : this.initial
   }
 
-  recall(action: ?Action) {
+  recall(action: ?Action): Object {
     if (action && action.id in this.snapshots) {
-      return this.snapshots[action.id]
+      return this.snapshots[action.id].next
     }
 
-    return this.initial
+    return this.getInitialState()
+  }
+
+  /**
+   * Get the state prior to a given action, but include any upstream
+   * updates from parent Microcosms.
+   */
+  rebase(action: Action): Object {
+    let state = this.recall(action.parent)
+
+    if (this.parent) {
+      state = merge(state, this.parent.recall(action))
+    }
+
+    return state
   }
 
   /**
    * Create the initial state snapshot for an action. This is important so
    * that, when rolling back to this action, it always has a state value.
    */
-  createSnapshot(action: Action) {
-    this.snapshots[action.id] = this.recall(action.parent)
+  createSnapshot(action: Action): Snapshot {
+    let snapshot: Snapshot = {
+      last: this.state,
+      next: this.state,
+      status: 'inactive',
+      payload: undefined
+    }
+
+    this.snapshots[action.id] = snapshot
+
+    return snapshot
   }
 
   /**
    * Update the state snapshot for a given action
    */
   updateSnapshot(action: Action) {
-    let next = this.recall(action.parent)
-
-    if (this.parent) {
-      next = merge(next, this.parent.recall(action))
-    }
+    let snap = this.snapshots[action.id]
+    let last = this.rebase(action)
 
     if (!action.disabled) {
-      next = this.domains.dispatch(next, action)
+      snap.next = this.domains.dispatch(action, last, snap)
+    } else {
+      snap.next = last
     }
 
-    this.state = this.snapshots[action.id] = next
+    this.snapshots[action.id] = merge(snap, {
+      last,
+      status: action.status,
+      payload: action.payload
+    })
+
+    this.state = snap.next
   }
 
   /**
diff --git a/src/utils.js b/src/utils.js
index 922b7dd6..f62e25a1 100644
--- a/src/utils.js
+++ b/src/utils.js
@@ -218,10 +218,3 @@ export function update(
 
   return set(state, path, next)
 }
-
-/**
- * A couple of methods use identity functions. This avoids duplication.
- */
-export function identity(n: *) {
-  return n
-}


Test patch:
diff --git a/test/integration/efficiency.test.js b/test/integration/efficiency.test.js
index 00cae5fc..ae1b8fcd 100644
--- a/test/integration/efficiency.test.js
+++ b/test/integration/efficiency.test.js
@@ -80,7 +80,7 @@ describe('Efficiency', function() {
     two.resolve()
     one.resolve()
 
-    expect(handler).toHaveBeenCalledTimes(3)
+    expect(handler).toHaveBeenCalledTimes(2)
   })
 
   it('does not dispatch a change event if nothing changes on the first reconciliation', () => {
diff --git a/test/integration/reexecuting-domain-handlers.test.js b/test/integration/reexecuting-domain-handlers.test.js
new file mode 100644
index 00000000..238a67b9
--- /dev/null
+++ b/test/integration/reexecuting-domain-handlers.test.js
@@ -0,0 +1,107 @@
+import Microcosm from '../../src/microcosm'
+
+describe('Re-executing domain handlers', function() {
+  it('does not re-invoke a domain handler if it would result in no change', function() {
+    let repo = new Microcosm()
+
+    let counterOneCalls = 0
+    let counterTwoCalls = 0
+
+    repo.addDomain('counterOne', {
+      getInitialState() {
+        return 0
+      },
+      add(count, n) {
+        counterOneCalls += 1
+        return count + n
+      },
+      register() {
+        return {
+          addOne: this.add
+        }
+      }
+    })
+
+    repo.addDomain('counterTwo', {
+      getInitialState() {
+        return 0
+      },
+      add(count, n) {
+        counterTwoCalls += 1
+        return count + n
+      },
+      register() {
+        return {
+          addTwo: this.add
+        }
+      }
+    })
+
+    let one = repo.append('addOne')
+
+    repo.push('addTwo', 2)
+
+    one.resolve(1)
+
+    expect(counterOneCalls).toBe(1)
+    expect(counterTwoCalls).toBe(1)
+    expect(repo.state).toEqual({ counterOne: 1, counterTwo: 2 })
+  })
+
+  it('domains in a fork do not invalidate the parent when both register the same action', function() {
+    let parent = new Microcosm()
+
+    let counterOneCalls = 0
+    let counterTwoCalls = 0
+
+    parent.addDomain('counterOne', {
+      getInitialState() {
+        return 0
+      },
+      add(count, n) {
+        counterOneCalls += 1
+
+        return count + n
+      },
+      register() {
+        return { test: this.add }
+      }
+    })
+
+    let child = parent.fork()
+
+    child.addDomain('counterTwo', {
+      getInitialState() {
+        return 0
+      },
+      add(count, n) {
+        counterTwoCalls += 1
+        return count + n
+      },
+      register() {
+        return { test: this.add }
+      }
+    })
+
+    let one = parent.append('test')
+
+    parent.push('test', 2)
+
+    one.resolve(1)
+
+    // The parent should get updates, but not state managed by a child
+    expect(parent).toHaveState('counterOne', 3)
+    expect(parent).not.toHaveState('counterTwo')
+
+    // The child gets upstream updates, but maintains its own state
+    expect(child).toHaveState('counterOne', 3)
+    expect(child).toHaveState('counterTwo', 3)
+
+    // There are two actions. We expect three calls:
+    // 1. two.resolve - one is pending, dispatch two
+    // 2. one.resolve - one completed, dispatch one
+    // 3. two.resolve - since one changed count, re-dispatch two
+    expect(counterOneCalls).toBe(3)
+    expect(counterTwoCalls).toBe(3)
+  })
+})
diff --git a/test/integration/rollbacks.test.js b/test/integration/rollbacks.test.js
index 667dbd60..f6e7e26f 100644
--- a/test/integration/rollbacks.test.js
+++ b/test/integration/rollbacks.test.js
@@ -24,8 +24,10 @@ describe('rollbacks', function() {
 
       register() {
         return {
-          [send.open]: this.addLoading,
-          [send.done]: this.add
+          [send]: {
+            open: this.addLoading,
+            done: this.add
+          }
         }
       }
     })
diff --git a/test/integration/snapshots-in-forks.test.js b/test/integration/snapshots-in-forks.test.js
new file mode 100644
index 00000000..24ae9f78
--- /dev/null
+++ b/test/integration/snapshots-in-forks.test.js
@@ -0,0 +1,44 @@
+import Microcosm from '../../src/microcosm'
+
+describe('Snapshots', function() {
+  it('forks accommodate actions that were pushed before the parent was forked', function() {
+    let parent = new Microcosm()
+
+    let action = parent.append('add', 'open')
+
+    parent.addDomain('top', {
+      getInitialState() {
+        return 0
+      },
+      add(count, n) {
+        return count + n
+      },
+      register() {
+        return {
+          add: this.add
+        }
+      }
+    })
+
+    let child = parent.fork()
+
+    child.addDomain('bottom', {
+      getInitialState() {
+        return 0
+      },
+      add(count, n) {
+        return count + n
+      },
+      register() {
+        return {
+          add: this.add
+        }
+      }
+    })
+
+    action.resolve(2)
+
+    expect(parent).toHaveState('top', 2)
+    expect(child).toHaveState('bottom', 2)
+  })
+})
diff --git a/test/unit/microcosm/addDomain.test.js b/test/unit/microcosm/addDomain.test.js
index 9d509789..8bafb9e9 100644
--- a/test/unit/microcosm/addDomain.test.js
+++ b/test/unit/microcosm/addDomain.test.js
@@ -75,5 +75,24 @@ describe('Microcosm::addDomain', function() {
       expect(repo).not.toHaveState('b')
       expect(fork).toHaveState('b', 2)
     })
+
+    it('adding a domain to a child does not modify a parent', function() {
+      let repo = new Microcosm()
+      let fork = repo.fork()
+
+      repo.addDomain('a', {
+        getInitialState() {
+          return 0
+        }
+      })
+
+      fork.addDomain('b', {
+        getInitialState() {
+          return 2
+        }
+      })
+
+      expect(repo).not.toHaveState('b')
+    })
   })
 })