870774 - Adopt Async.jsm as Toolkit module

Status: RESOLVED WONTFIX

(Toolkit :: General, defect)

Description

[Mike de Boer [:mikedeboer]]

For a full explanation, background & details: please check out https://gist.github.com/mikedeboer/5495405 and the mailing list discussions on dev-platform and dev-firefox.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Add Async.jsm, including tests

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Add Async.jsm, including tests

Unbitrotting and Mozilla-fying the code

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 761360 [details] [diff] [review]
Add Async.jsm, including tests

Adding Richard as a 2nd reviewer because this patch deserves many eyes.

Comment 4

[Gregory Szorc [:gps]]

And Toolkit is the proper place for this bug...

Comment 5

[Gregory Szorc [:gps]]

I'm kinda interested in Dave Townsend's opinion on this. I wonder if Add-on SDK already has something similar...

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 761360 [details] [diff] [review]
Add Async.jsm, including tests

Review of attachment 761360 [details] [diff] [review]:
-----------------------------------------------------------------

I've gone too long without food to finish this review at the moment. But, I thought you'd appreciate partial feedback.

I like having this in the tree. However, before a comprehensive review can be performed, I'd really like to see proper docs. As I'm reading the code, I'm having a hard time understanding exactly what each function should do. I see there are docs at https://github.com/caolan/async, but that's not good enough: we should have something in tree.

I worry a little about excessive use of functions. There are a handful of places where IMO we could reuse an existing function instead of creating a new one for every invocation. Useful test: if there is no state inside a function instance/closure, you don't need to be creating separate function instances/closures.

Leaving review open for completion later.

::: toolkit/modules/Async.jsm
@@ +15,5 @@
> +this.EXPORTED_SYMBOLS = [
> +  "Async"
> +];
> +
> +this.Async = (function() {

Why not just assign an object directly?

@@ +24,5 @@
> +  let slice = Array.prototype.slice;
> +
> +  function only_once(fn) {
> +    let called = false;
> +    return function() {

Nit: I think most of us put a space between "function" and the parens. We also typically name fully anonymous functions so stack traces are prettier.

@@ +25,5 @@
> +
> +  function only_once(fn) {
> +    let called = false;
> +    return function() {
> +      if (called)

Nit: Always use braces. Throughout the file.

@@ +38,5 @@
> +  async.setImmediate = function(fn, delay) {
> +    delay = delay || 0;
> +    let timer = Components.classes["@mozilla.org/timer;1"]
> +                          .createInstance(Components.interfaces.nsITimer);
> +    timer.initWithCallback(fn, delay, Components.interfaces.nsITimer.TYPE_ONE_SHOT);

I /think/ that you need to take additional action on nsITimer instances (such as calling .cancel()) or deleting the instance explicitly to prevent it from lingering. I see there is now a Timer.jsm in /toolkit/modules you can use.

@@ +47,5 @@
> +    if (!timer)
> +      return;
> +    try {
> +      timer.cancel();
> +    } catch (ex) {}

Since you catch everything, I'm tempted to say it's OK to remove the !timer check.

@@ +50,5 @@
> +      timer.cancel();
> +    } catch (ex) {}
> +  }
> +
> +  async.each = function(arr, iterator, callback) {

Please document this and other functions!

@@ +56,5 @@
> +    if (!arr.length) {
> +      return callback();
> +    }
> +    let completed = 0;
> +    arr.forEach(function(x) {

So, we're limiting arr to be an array. IMO it would be nice if this function supported any object that supported the iterator interface. I reckon that could be a follow-up bug.

@@ +70,5 @@
> +        }
> +      }));
> +    });
> +  };
> +  async.forEach = async.each;

I tend to not like aliasing because it increases cognitive dissonance. "What's the difference between each and forEach?"

@@ +72,5 @@
> +    });
> +  };
> +  async.forEach = async.each;
> +
> +  async.eachSeries = function(arr, iterator, callback) {

Docs! I'm not sure what this is supposed to do or how it varies from forEach. I'm too lazy to infer it from the code or the tests, so I'm not reviewing it for now.

@@ +103,5 @@
> +    fn.apply(null, [arr, iterator, callback]);
> +  };
> +  async.forEachLimit = async.eachLimit;
> +
> +  var _eachLimit = function(limit) {

let!

@@ +656,5 @@
> +    return q;
> +  };
> +
> +  async.cargo = function(worker, payload) {
> +      let working = false;

Nit: 4 space indent?!

@@ +723,5 @@
> +    };
> +    return cargo;
> +  };
> +
> +  async.log = function(fn, ...args) {

Does this have any value? Perhaps we should wait on log4moz to land in toolkit.

Comment 7

[Mike de Boer [:mikedeboer]]

Greg, I think this is more than enough for a first pass - thanks! It's already a LOT to ask to review this in one go - the module comprises more than three years community development effort, thus inherited its quirks as well...

I will work through your initial batch of comments tomorrow and see where we're at then... good idea?

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 761360 [details] [diff] [review]
Add Async.jsm, including tests

Review of attachment 761360 [details] [diff] [review]:
-----------------------------------------------------------------

I generated a few review comments, so stopped half way through to wait for another draft.

My overall comments:

* I'm concerned about the perf characteristics here. Many of these methods produce a lot of garbage; all of those easy slice/splice/function() things that you're using produce more junk than you might realize. That's fine in test code, or in low-throughput areas of the codebase, but if we're dumping this stuff into the tree and crying "have at!", we should be sure to do two things: address perf proactively; and document the hell out of this. Someone's going to use this to handle ten thousand history items, and our users are going to suffer from 3GB of heap getting GCed as you allocate fifty thousand objects and make fifty thousand function calls. I want to make sure that we don't end up in the position of the perf team forcing some poor schmuck to rewrite his code to *not* use this library.

* There are limits here. You use apply, for example. What if I call this with a million-item array? Do I blow out the max arg length? You need to document these limits or work around them.

* This is definitely dev-doc-needed, probably prior to landing.

* I question the inclusion of some of this thousand-line functional programming toolchest (and as a Lisper, I miss many of these things myself, so that's hard for me to say!). There are costs to shipping code -- testing, doc maintenance, runtime memory usage, package size, assorted measures of complexity. Why do we need to ship `forever`? What do we need an async `times` function for? Is anyone really going to incur the cost of importing this whole module just to get a convenience method like `times`?

* The module name collides with services/common/async.js.

So next steps IMO: you need to figure out what the memory footprint is of this module, and then if it's big make a case for why we're happy shipping that on b2g, on low-end ARMv6 Android devices, and overall in the competitive marketplace. Maybe the answer is to consolidate the existing code in the tree that implements all this stuff, and only land the bits we need. If there's nothing in Firefox's three million lines of code that needs all of this, even speculatively, then maybe we shouldn't land it yet at all.

::: toolkit/modules/Async.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * This module is a direct port of the awesome Async library by Caolan McMahon
> + * and contributors. License: MIT. More information can be found at 

Trailin' whitespace.

@@ +34,5 @@
> +  }
> +
> +  // Exported async module functions
> +
> +  async.setImmediate = function(fn, delay) {

You definitely want to document these. For example, the caller of this function should retain a reference to the timer.

@@ +50,5 @@
> +      timer.cancel();
> +    } catch (ex) {}
> +  }
> +
> +  async.each = function(arr, iterator, callback) {

Seconded. What is iterator? It sure doesn't look like an iterator to me. It's a function that takes an item and a callback, so perhaps 'action'?

@@ +60,5 @@
> +    arr.forEach(function(x) {
> +      iterator(x, only_once(function(err) {
> +        if (err) {
> +          callback(err);
> +          callback = function() {};

My nose says that this is needlessly complicated. only_once seems needlessly defensive (or, rather, very expensive to guard against some real edge cases).

You should define a single no-op function in an enclosing scope, and use it here instead of a new function.

Incidentally, this is one of my favorite interview questions :P

@@ +63,5 @@
> +          callback(err);
> +          callback = function() {};
> +        } else {
> +          completed += 1;
> +          if (completed >= arr.length) {

let remaining = arr.length;

then count down instead. Saves you an array length check on every callback.

@@ +72,5 @@
> +    });
> +  };
> +  async.forEach = async.each;
> +
> +  async.eachSeries = function(arr, iterator, callback) {

I think it's supposed to be `eachSequential` -- chaining.

@@ +73,5 @@
> +  };
> +  async.forEach = async.each;
> +
> +  async.eachSeries = function(arr, iterator, callback) {
> +    callback = callback || function() {};

I would prefer to not allow callers to do something stupid like this. If they want to do nothing on completion, let them pass in an empty function.

@@ +83,5 @@
> +      iterator(arr[completed], function(err) {
> +        if (err) {
> +          callback(err);
> +          callback = function() {};
> +        } else {

Prefer early return to else.

@@ +88,5 @@
> +          completed += 1;
> +          if (completed >= arr.length) {
> +            callback(null);
> +          } else {
> +            iterate();

Holy stack depth, Batman. :P

@@ +97,5 @@
> +    iterate();
> +  };
> +  async.forEachSeries = async.eachSeries;
> +
> +  async.eachLimit = function(arr, limit, iterator, callback) {

I'd call this `eachConstrained`, because `limit` implies that you're processing fewer items from `arr`.

@@ +143,5 @@
> +
> +  var doParallel = function(fn) {
> +    return function(...args) {
> +      return fn.apply(null, [async.each].concat(args));
> +    };

*gets the perf twitches*

Comment 9

[Mike de Boer [:mikedeboer]]

Richard, thanks for the awesome review, I'll post a follow-up as soon as I can!

Comment 10

[Dave Townsend [:mossop]]

My worry here is that this is a complex solution looking for a problem. Ignoring the test harness (if that was all we cared about then we'd just land this in the test harness) what use are we expecting to make of this?

I'd much prefer to land this piecemeal, as and when a need arises for particular parts rather than taking it wholesale.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 761360 [details] [diff] [review]
Add Async.jsm, including tests

Please re-tag me when you've made follow-ups

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 761360 [details] [diff] [review]
Add Async.jsm, including tests

(Getting this out of my review queue...)