Add batching #295

nhunzaker · 2017-04-12T17:08:52Z

This commit exposes control over the release process so that changes can be batched together. It also ensures that this does not break history.wait().

Additionally, it updates the canvas example to use a requestIdleCallback batching strategy. This improves write throughput about 60-80%, but I'm mostly looking forward to preventing duplicate renders for loading states that finish really quickly.

http://probable-pocket.surge.sh/

codecov-io · 2017-04-12T17:09:00Z

Codecov Report

Merging #295 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #295   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          22     23    +1     
  Lines         802    820   +18     
=====================================
+ Hits          802    820   +18

Impacted Files	Coverage Δ
src/history.js	`100% <100%> (ø)`	⬆️
src/default-update-strategy.js	`100% <100%> (ø)`
src/microcosm.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19ee6ec...a10c30b. Read the comment docs.

nhunzaker · 2017-04-13T11:48:03Z

examples/canvas/app/updater.js

+      }, options)
+    }
+  }
+}


It would be cool for this to be a microcosm-batch-updates module, or something. I imagine there's really only one way to do this.

cwmanning · 2017-04-13T11:58:13Z

docs/api/microcosm.md

-Microcosm is [Flux](https://facebook.github.io/flux/) with first-class
-actions and state sandboxing.
+The Microcosm class provides a centralized place to store application
+state, dispatch actions, and track changes.


I am wondering if we should keep the reference to Flux for the "This is like X" aspect of documentation, otherwise this is an awesome change. 👍

cwmanning · 2017-04-13T12:05:51Z

docs/api/microcosm.md

+you provide to Microcosm are passed into the `setup` lifecycle method:
+
+```javascript
+import Autosave from './effects/autosave'


can this example code demonstrate the concept without introducing effects? to keep the section as simple as possible

Hmm. Good idea.

cwmanning · 2017-04-13T12:11:25Z

docs/recipes/batch-updates.md

@@ -0,0 +1,132 @@
+# Batch Updates


this doc is really good, 👏

cwmanning · 2017-04-13T12:18:34Z

src/history.js

 /**
 * The central tree data structure that is used to calculate state for
 * a Microcosm. Each node in the tree is an action. Branches are
 * changes over time.
 * @constructor
 */
-export default function History (limit) {
+export default function History (config) {


with this change, would it make sense to warn when config is not an object?

I don't think so. History is never instantiated directly, it always passes through new Microcosm()

oh I see, sounds good. somehow I thought I was looking at the Microcosm constructor

cwmanning

👍

nhunzaker · 2017-04-13T13:05:11Z

@cwmanning good call on the test. I discovered a bug where you could get stuck in wait() if you waited during a release.

Which made me realize... we don't need to request any more "updates" if the last update hasn't fired yet. That can reduce the complexity of the updater significantly.

Instead of doing:

// A lot of browsers don't support requestIdleCallback, so we patch it
import 'ric'

// Never let the user wait more than 24 milliseconds for an update
const options = { timeout: 24 }

export default function requestIdleBatch () {

  // Batching strategies return a function. This allows you to
  // maintain state within the closure above. Here, we keep track of
  // the last frame of work
  let frame = null

  return update => {
    if (frame == null) {
      frame = requestIdleCallback(() => {
        frame = null
        update()
      }, options)
    }
  }
}

We could just do:

// A lot of browsers don't support requestIdleCallback, so we patch it
import 'ric'

// Never let the user wait more than 24 milliseconds for an update
const options = { timeout: 24 }

export default function requestIdleBatch () {
  return update => requestIdleCallback(update, options)
}

I still want to return a function, because it would allow you to do things like setup logging or some other system. But you don't have to keep track of prior frames, which is really nice.

What do you think?

nhunzaker · 2017-04-13T13:08:12Z

@cwmanning technically now, you could also just do:

export default function requestIdleBatch () {
  return update => setTimeout(update, 24)
}

cwmanning · 2017-04-13T13:21:52Z

@nhunzaker I like the simplified updater example. Might include the really simple one (setTimeout) in the example code as well, for those who may opt to avoid a polyfill.

nhunzaker · 2017-04-13T14:07:12Z

@cwmanning Done, what do you think?

nhunzaker · 2017-04-13T14:08:11Z

@leobauza Do these documentation updates feel good? Anything we should change?

cwmanning

👍

efatsi · 2017-04-13T15:17:24Z

Neat stuff, curious what peoples' thoughts are around simplifying the interface for this, and just exposing a configurable batch delay option that would instantiate an in-house updater. Reading through the Batch Updates docs is neat, but by the end of the docs I got the sense that there's really an optimal way to do things (use requestIdleCallback). Worth flushing this out a little more and replacing the idea of defining your own updater with just defining a batch interval? Or are there other benefits to defining your own updater that I'm not picking up on.

efatsi · 2017-04-13T15:13:10Z

docs/recipes/batch-updates.md

+test("it increases the number when the stepper is clicked", function () {
+  let app = mount(<App />)
+
+  app.find('#stepper).simulate('click')


should be app.find('#stepper') (missing closing quote mark after #stepper)

mackermedia · 2017-04-13T17:13:38Z

👍 Those docs are 💯 . Also, I think @efatsi is on to something with providing a simple mechanism to use the default recommended batch update operation.

nhunzaker · 2017-04-13T17:16:53Z

@mackermedia @efatsi Hmm, what do you think about making this option batch: true, and if it's a function, use that function instead?

nhunzaker · 2017-04-13T17:17:13Z

batch:true with the caveat that you'd need to polyfill requestIdleCallback

mackermedia · 2017-04-13T17:20:36Z

i was thinking batch: true would do the ric method and you wouldn't have to think about polyfilling (microcosm happily does that for you).
or maybe also if batch: 12 is a number it'll do the setTimeout method? is there any value to that method and if so, it might be nice for it to be a simple config option.
or you could also provide your own function?
overboard?

efatsi · 2017-04-13T17:27:14Z

agreeing with @mackermedia, passing in functions allows for greater control but requires more underlying knowledge of microcosm, mainly - what's passed in to that function. My thinking would be something like batchInterval: 12.

leobauza

👍

leobauza · 2017-04-14T13:11:05Z

docs/api/microcosm.md

-Microcosm is [Flux](https://facebook.github.io/flux/) with first-class
-actions and state sandboxing.
+The Microcosm class provides a centralized place to store application
+state, dispatch actions, and track changes.


I am wondering if we should keep the reference to Flux for the "This is like X" aspect of documentation, otherwise this is an awesome change. 👍

nhunzaker · 2017-04-14T15:05:10Z

@efatsi, @mackermedia Here's my thinking. I want a safe default, with an escape hatch. I think we can do this without exposing a lot of complexity.

I think we need to add 2-3 options:

batch: true
batchInterval: 24, // ? Maybe... is this useful?
updater: buildInUpdater

The updater function will get the options passed into Microcosm. Our default updater function would look like:

function requestIdleBatch (options) {
  if (options.batch) {
    return update => requestIdleCallback(update, { timeout: options.batchInterval })
  } else {
    return update => update()
  }
}

This allows you:

To just set batch: true. End of story
To configure the batch interval (is it an interval, or a timeout?)
To configure a custom updater.
To conditionally apply that batch updater based on environment

I'm still debating whether or not we need batchInterval. I think a timeout of 24 would be exactly what I would do on any given project, and tweaking that timeout under the hood wouldn't be a breaking change (unless we go from like 24ms to 200ms).

What do you think?

mackermedia · 2017-04-14T15:10:50Z

Honestly, I don't really know of the use-case of when I'd reach for it. It sounds like when the app starts to get janky from a lot of actions resolving and re-rendering when I want an animation to finish.
I think the batch: true and updater: myUpdateFunction are probably all that's necessary for now?
Keep it simple?

nhunzaker · 2017-04-14T15:59:16Z

@mackermedia Yep. That's precisely the use case. When you have a lot of actions firing, causing meaningful changes, and the successive change events cause expensive repaints.

Just having batch and updater sound good to me.

efatsi · 2017-04-14T17:08:34Z

👍 to batch and updater, and then to this PR

nhunzaker · 2017-04-14T20:06:59Z

batch and updater it is.

This commit exposes control over the release process so that changes can be batched together. It also ensures that this does not break `history.wait()`. Additionally, it updates the canvas example to use a requestIdleCallback batching strategy.

nhunzaker force-pushed the batching branch from b3eb05a to 200409b Compare April 12, 2017 17:09

nhunzaker commented Apr 13, 2017

View reviewed changes

nhunzaker force-pushed the initial-state-update branch from 2a24bcd to 1435301 Compare April 13, 2017 11:56

nhunzaker force-pushed the batching branch from 5231060 to de7f6c1 Compare April 13, 2017 11:56

nhunzaker changed the base branch from initial-state-update to master April 13, 2017 11:56

cwmanning reviewed Apr 13, 2017

View reviewed changes

cwmanning approved these changes Apr 13, 2017

View reviewed changes

nhunzaker self-assigned this Apr 13, 2017

nhunzaker requested a review from leobauza April 13, 2017 14:07

cwmanning approved these changes Apr 13, 2017

View reviewed changes

efatsi reviewed Apr 13, 2017

View reviewed changes

leobauza approved these changes Apr 14, 2017

View reviewed changes

nhunzaker added 17 commits April 14, 2017 16:07

Add batching

06aad19

This commit exposes control over the release process so that changes can be batched together. It also ensures that this does not break `history.wait()`. Additionally, it updates the canvas example to use a requestIdleCallback batching strategy.

Update docs to include information on batch updates

4526367

Tweak Microcosm overview

d97da73

Tweak options section of microcosm.md

8f8f57a

Tweak options descriptions

01ac191

Restructure batching to avoid burying solution

05a04f7

Tweak batching recipe

e6eb3fd

Tweak batch-updates.md

5916c27

Fix case where wait() would get stuck between queued updates

5759905

Do not queue no releases if an old release hasn't finished

7ec4da4

Update updater in canvas example and batch updates doc with simpler form

e16c8ac

Introduce setTimeout example first

443eec0

Fix un-terminated string in batch example

d9eeaee

Set default batching function

e4d11d0

Simplify option passing docs

410bc7d

Update batch docs, add test for default updater

06f7b17

Remove ric from devDependencies

0428285

nhunzaker force-pushed the batching branch from f171683 to 0428285 Compare April 14, 2017 20:10

Bump webpack

a10c30b

nhunzaker merged commit a10c30b into master Apr 14, 2017

nhunzaker deleted the batching branch April 17, 2017 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add batching #295

Add batching #295

nhunzaker commented Apr 12, 2017

codecov-io commented Apr 12, 2017 •

edited

nhunzaker Apr 13, 2017 •

edited

cwmanning Apr 13, 2017

leobauza Apr 14, 2017

cwmanning Apr 13, 2017

nhunzaker Apr 13, 2017

cwmanning Apr 13, 2017

cwmanning Apr 13, 2017

nhunzaker Apr 13, 2017

cwmanning Apr 13, 2017

cwmanning left a comment

nhunzaker commented Apr 13, 2017 •

edited by cwmanning

nhunzaker commented Apr 13, 2017

cwmanning commented Apr 13, 2017

nhunzaker commented Apr 13, 2017

nhunzaker commented Apr 13, 2017

cwmanning left a comment

efatsi commented Apr 13, 2017

efatsi Apr 13, 2017

nhunzaker Apr 13, 2017

mackermedia commented Apr 13, 2017

nhunzaker commented Apr 13, 2017

nhunzaker commented Apr 13, 2017 •

edited

mackermedia commented Apr 13, 2017

efatsi commented Apr 13, 2017

leobauza left a comment

leobauza Apr 14, 2017

nhunzaker commented Apr 14, 2017

mackermedia commented Apr 14, 2017

nhunzaker commented Apr 14, 2017

efatsi commented Apr 14, 2017

nhunzaker commented Apr 14, 2017

Add batching #295

Add batching #295

Conversation

nhunzaker commented Apr 12, 2017

codecov-io commented Apr 12, 2017 • edited

Codecov Report

nhunzaker Apr 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwmanning left a comment

Choose a reason for hiding this comment

nhunzaker commented Apr 13, 2017 • edited by cwmanning

nhunzaker commented Apr 13, 2017

cwmanning commented Apr 13, 2017

nhunzaker commented Apr 13, 2017

nhunzaker commented Apr 13, 2017

cwmanning left a comment

Choose a reason for hiding this comment

efatsi commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mackermedia commented Apr 13, 2017

nhunzaker commented Apr 13, 2017

nhunzaker commented Apr 13, 2017 • edited

mackermedia commented Apr 13, 2017

efatsi commented Apr 13, 2017

leobauza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Apr 14, 2017

mackermedia commented Apr 14, 2017

nhunzaker commented Apr 14, 2017

efatsi commented Apr 14, 2017

nhunzaker commented Apr 14, 2017

codecov-io commented Apr 12, 2017 •

edited

nhunzaker Apr 13, 2017 •

edited

nhunzaker commented Apr 13, 2017 •

edited by cwmanning

nhunzaker commented Apr 13, 2017 •

edited