692420 - (JSScheduleAPI) Investigate a proper mechanism for queuing initialization tasks

Status: RESOLVED WONTFIX

(Core :: General, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

We have moved many initialization tasks away from the critical startup path. However, a number of these tasks are triggered by 5 seconds timers, which is both fragile and difficult to test - and which bites users whose startup lasts more than 5 seconds, typically on Android.

We should investigate a good method to handle such tasks.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

I believe that we have the following usage scenarios.

1/ I have an initialization task A and I want it to happen once critical startup is complete. -- this is certainly the most common task.
2/ I have a task (initialization or non-initialization) B and I want it to happen, but only after task A is complete.
3/ I have an initialization task A and I want to background it.
4/ I want to know if task A is complete.

As an added bonus, for developer convenience
5/ I have a task A, I don't really know when I want to execute it, but I'd like to experiment easily.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

See https://wiki.mozilla.org/User:Yoric/RunLevelQueue for one proposal on this topic.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: A list of all nsITimer created at startup

I attach a list of all timers created at startup. Depending on runs, I get between 340 and 390. Some of them are triggered by layout. Most of the others sound like candidates for queuing.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Somewhat human-readable list of nsITimers created at startup

Same list, but with an attempt to parse the results and turn them into something somewhat human-readable.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fully human-readable list of nsITimers created at startup

I attach a list of nsITimers created at startup on Fennec, with a non-trivial profile, and this time, it should be human-readable.

If the measure is correct, it seems that we are down to 30 timers, against 340+ for desktop Firefox.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fully human-readable list of nsITimers created at startup, on a non-trivial profile, Fennec

Same measure, but with a non-trivial profile. This time, I have 131 calls.

Comment 7

[(dormant account)]

Comment on attachment 566488 [details]
Fully human-readable list of nsITimers created at startup, on a non-trivial profile, Fennec

This is really good stuff. Thanks!

Comment 8

[David Teller [:Yoric] - still alive but not very active]

A few other candidates (not all of which are really initialization tasks):
- nsDownloadManager::RemoveDownloadsForURI
- AddonDatabase.insertAddons
- anything that sets places annotations.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Recent changes to Chrome Workers have made it much less convenient to have a non-main-thread JavaScript work queue - we cannot send closures or mutable values between threads, we cannot call XPCom from non-main thread.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

I have an early prototype for JS.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: A proposal

I attach the draft of a mechanism for solving this problem. As very close variants of this problem are encountered by e10s and Jetpack, some feedback would be interesting from these teams, too.

Note: The performance team is interested in making the code more asynchronous and in a good mechanism to ensure that initializations happen as late as possible.

Note: From what I understand, e10s and Jetpack are interested in getting processes to communicate and in a good mechanism to transmit streams of values from one side to the other.

This tiny library offers two constructions: |Async| and |Schedule|.

An |Async| is simply an object that can be used for asynchronous communications, whether these values are unique (e.g. returning from a function) or not (e.g. streams). They can transmit successful results, negative results and stoppers (e.g. stream closing).

Global object |Schedule| offers a few functions to asynchronously launch functions and capture their result in an |Async|, or to share system-wide |Async|s.

Barring any issue, I intend to extend |Schedule| to implement a facility for limited communication with threads, and to use it for backgrounding some parts of the initialization of Firefox.

For more info, see the documentation in the attached file.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Asynchronous communications

Dividing the patch in two, for easier reading.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Scheduling mechanism

Comment 14

[(dormant account)]

Comment on attachment 578310 [details]
Asynchronous communications

I think by "success" you mean "return value". r- unclear naming, but general idea seems sound.

I'm also not clear from your doc comment on how to use this.
async = new Async()
async.(sucess|error) = foo
then?
async.onsuccess(async)?

Comment 15

[(dormant account)]

Comment on attachment 578311 [details]
Scheduling mechanism

Your names read/sound nicely, however naming should reflect the behavior. I should not have to look in docs as what the difference between soon/politely/immediately

I would get rid of soon, politely instead have an explicit Schedule.soon(timeout). Sometimes it's nice to say "i want something to run, but no sooner than 100ms from now". When no parameters are specified it would do mainThread.dispatch.

I think immediately is a risky function. For a convenience func I would call it Schedule.execute() it's only an aid to help incrementally refactor js. It shouldn't look like a weird alternative to .soon.

Schedule.bg -> Schedule.worker..I think that's what you mean by thread.

Rest of this looks nice. The main missing part is categorization. I don't expect a user to have similar tasks to queue up, however a user is likely to queue up a category of tasks.

ie if someone does Schedule.worker(Schedule.DISK), this should mean that even though an operation is scheduled async, it should run sync with respect to other operations that were scheduled via Schedule.worker(Schedule.DISK). Should also think about how to schedule tasks from C++, since those also need to cooperate with ones from JS.
Eg, how do you schedule a netwerk cache purge operation? In C++ we will have to have some operations run on main thread..those should also delay operations scheduled with Schedule.DISK to avoid thrashing the disk.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #14)
> Comment on attachment 578310 [details]
> Asynchronous communications
> 
> I think by "success" you mean "return value". r- unclear naming, but general
> idea seems sound.
> 
> I'm also not clear from your doc comment on how to use this.
> async = new Async()
> async.(sucess|error) = foo
> then?
> async.onsuccess(async)?

Thanks for the feedback. Turns out that we already have a library of promises in devtools, so I am refactoring my code to use their library instead of mine.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #15)
> Comment on attachment 578311 [details]
> Scheduling mechanism
> 
> Your names read/sound nicely, however naming should reflect the behavior. I
> should not have to look in docs as what the difference between
> soon/politely/immediately
> 
> I would get rid of soon, politely instead have an explicit
> Schedule.soon(timeout). Sometimes it's nice to say "i want something to run,
> but no sooner than 100ms from now". When no parameters are specified it
> would do mainThread.dispatch.

Good idea.

> I think immediately is a risky function. For a convenience func I would call
> it Schedule.execute() it's only an aid to help incrementally refactor js. It
> shouldn't look like a weird alternative to .soon.

It is now |Schedule.executeImmediately| - this should avoid any ambiguity.

> Schedule.bg -> Schedule.worker..I think that's what you mean by thread.

It is, and good idea.

> 
> Rest of this looks nice. The main missing part is categorization. I don't
> expect a user to have similar tasks to queue up, however a user is likely to
> queue up a category of tasks.
> 
> ie if someone does Schedule.worker(Schedule.DISK), this should mean that
> even though an operation is scheduled async, it should run sync with respect
> to other operations that were scheduled via Schedule.worker(Schedule.DISK).

Good point. Do you think I should do this also for |Schedule.later| or just for |Schedule.worker|? I believe that the only real difficulty will be to synchronize this between several threads who might otherwise be executing |Schedule.DISK|
operations concurrently.

> Should also think about how to schedule tasks from C++, since those also
> need to cooperate with ones from JS.
> Eg, how do you schedule a netwerk cache purge operation? In C++ we will have
> to have some operations run on main thread..those should also delay
> operations scheduled with Schedule.DISK to avoid thrashing the disk.

I am not completely sure how I will cooperate with C++ yet. Wrapping this JS code into C++ through JSAPI (or XPConnect, if it were still possible) sounds like a good way to introduce bad race conditions.

If it is ok, I will |Schedule.work| that issue to the back of my mind, and only add this feature to a v2.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: v2. Schedule

I attach a second version of module |Schedule|, with the following changes:
- now uses devtools library Promises.jsm;
- collapsed |Schedule.soon| and |Schedule.later|, as requested, did the same for |Promise.soon|/|Promise.later|, |Schedule.execute.soon|/|Schedule.execute.later|;
- added support for categories to all these functions, and added one sample category |Category.DISK|;
- moved |Private.execute| into |Schedule.execute|, as it turns out that these functions are useful in other contexts (I am using them in |OS.File|);
- renamed |Schedule.immediately| into |Schedule.executeImmediately|, renamed |Schedule.execute.immediately| into |Schedule.execute.executeImmediately|;
- updated documentation accordingly.

I attach the testsuite in another patch.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: v2. Schedule

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: v2. Test suite

Companion test.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

With this, I should have enough to [re]implement OS.File async operations. I believe that this module is now complete enough to be useful and could – pending reviews – be pushed without any additional features.

Alternatively, I can start working on function |Schedule.worker|. I believe that I have a clear idea of how I can implement it.

For interaction with C++, I now have a better idea, too. I believe that the best way to do this is to have completely separate C++ and JavaScript threads and to synchronize only through categories. This would require reimplementing categories with either js-ctypes or JSAPI. This sounds reasonably easy, although I would prefer first implementing (and landing) a version that implements |Schedule.worker|.

Taras, what do you think?

Comment 22

[Joe Walker [:jwalker]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> Created attachment 580006 [details] [diff] [review]
> v2. Schedule
> 
> I attach a second version of module |Schedule|, with the following changes:
> - now uses devtools library Promises.jsm;

Please could we think about moving this to somewhere common? In many ways, creating a hack for limtted use is a smaller step than promoting it to being a shared module. So I think we should do this with our eyes open.

OK for me to raise a bug for this?

Comment 23

[David Teller [:Yoric] - still alive but not very active]

(In reply to Joe Walker from comment #22)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> > Created attachment 580006 [details] [diff] [review]
> > v2. Schedule
> > 
> > I attach a second version of module |Schedule|, with the following changes:
> > - now uses devtools library Promises.jsm;
> 
> Please could we think about moving this to somewhere common? In many ways,
> creating a hack for limtted use is a smaller step than promoting it to being
> a shared module. So I think we should do this with our eyes open.
> 
> OK for me to raise a bug for this?

Are you talking about Promises.jsm? If so, I am all for it (and make the new bug a blocker for this one).

If we are talking about Schedule, the objective is definitely to make it shared. The code will probably migrate to toolkit/ and remain accessible as resource://gre/modules/schedule.jsm .

Comment 24

[Joe Walker [:jwalker]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #23)
> (In reply to Joe Walker from comment #22)
> > (In reply to David Rajchenbach Teller [:Yoric] from comment #18)
> > > Created attachment 580006 [details] [diff] [review]
> > > v2. Schedule
> > > 
> > > I attach a second version of module |Schedule|, with the following changes:
> > > - now uses devtools library Promises.jsm;
> > 
> > Please could we think about moving this to somewhere common? In many ways,
> > creating a hack for limtted use is a smaller step than promoting it to being
> > a shared module. So I think we should do this with our eyes open.
> > 
> > OK for me to raise a bug for this?
> 
> Are you talking about Promises.jsm? If so, I am all for it (and make the new
> bug a blocker for this one).

I was, and I did - bug 708984.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: v3. Module Schedule

A little clean-up on optional arguments. Added some guards, for better robustness. Removed Schedule.DISK, as experience shows it leads very easily to deadlocks.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: v3. Module Schedule

Updated the tests.

Comment 27

[(dormant account)]

I think Schedule.DISK queueing can be done by polling. ie set a variable somewhere to indicate category of worker task currently executing...ie have
Schedule.currentTask = null if nothing is executing, schedule.currentTask.Disk = {name:"task name"} if something is executing. 

Then the worker can reschedule itself if currentTask is not null and is already executing something. Clearly you'd need some sort of atomic test/set operation to avoid having 2 tasks running in parallel, but I think the risk of that is small and can be done in a followup.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #27)
> I think Schedule.DISK queueing can be done by polling. ie set a variable
> somewhere to indicate category of worker task currently executing...ie have
> Schedule.currentTask = null if nothing is executing,
> schedule.currentTask.Disk = {name:"task name"} if something is executing. 

This might not be possible: you can have several tasks of different categories executing simultaneously.

Have you looked at the current non-polling implementation, though? I moved it from Schedule to OS.File as attempting to use it outside of OS.File is a clear invitation to deadlock, but otherwise, it works.

> Then the worker can reschedule itself if currentTask is not null and is
> already executing something. Clearly you'd need some sort of atomic test/set
> operation to avoid having 2 tasks running in parallel, but I think the risk
> of that is small and can be done in a followup.

In JavaScript, I would not worry about atomicity: two threads simply cannot share mutable state.

Comment 29

[Dave Herman [:dherman]]

> Added some guards, for better robustness.

I don't see guards.jsm -- did you forget to hg add?

Dave

Comment 30

[David Teller [:Yoric] - still alive but not very active]

(In reply to Dave Herman [:dherman] from comment #29)
> > Added some guards, for better robustness.
> 
> I don't see guards.jsm -- did you forget to hg add?
> 
> Dave

Ah, you are right, they were lost somewhere in another bug. I separated them and added them as Bug 710139.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: v4. Module Schedule

Essentially the same code, with a little additional documentation, without using |var| and with additional tests. Writing an asynchronous, non-blocking, implementation of Fibonacci in 12 loc is nice.

Comment 32

[Dave Herman [:dherman]]

Comment on attachment 581599 [details] [diff] [review]
v4. Module Schedule

>+ * - schedule immediately a bunch of code for execution with a call such as
>+ *   Schedule.soon(function() { ... })
>+ *
>+ * - add a delay before execution
>+ *   Schedule.soon({delay: 100}, function() { ... })
>+ *
>+ * - group tasks by categories to ensure execution order and avoid simultanous/
>+ *   conflicting use of resources:
>+ *   Schedule.soon({category: DISK}, function() { ... })

I think you got rid of DISK so you should get rid of this example. Did you intend to get rid of categories altogether for now, too?

>+const Schedule = {
>+  /**
>+   * Schedule a task for asynchronous execution.
>+   *
>+   * Simple example:
>+   *  Schedule.soon(function(){ alert("I have been scheduled") } );
>+   * or, equivalently,
>+   *  Schedule.soon(alert, "I have been scheduled");

Also mention the ability to pass arguments for the thunk in the Schedule JavaDoc comment too?

>+   * @param {{delay:number|undefined,
>+   *         category:Schedule.Category|undefined,

No more category?

>+   *         args:Array|undefined}=} aOptions A set of options, as an object
>+   *   which may have fields "delay" (a number of milliseconds to wait before
>+   *   executing), "category" (an optional category for the action - the
>+   *   scheduler will enforce ordering between actions of the same category
>+   *   and ensure that two actions of the same category are not executed
>+   *   concurrently), "args" (a set of args to pass to the task).
>+   * @param {Function|*} aTask Either a function or an object
>+   * with a method |apply|.
>+   * @param {...*} aArguments Arguments to pass to |aTask|. If |aOptions.args|
>+   * is present, these arguments will be appended after |aOptions.args|.
>+   * @return {Promise} An object that may be used to watch the execution
>+   * of the task and obtain its result.
>+   */
>+  soon: function Schedule_soon(aOptions, aTask) {

I don't understand the naming convention (the "a" prefix). Is this a Mozilla convention I'm not familiar with?

>+    let args;
>+    //1. Handle optional argument |aOptions|

Space between "//" and start of comment.

>+    if (aOptions.apply) {
>+      args = Array.prototype.slice.call(arguments);
>+      args.shift();//Remove one arg

Space before the "//" and between "//" and start of comment.

>+      aTask    = aOptions;

Unnecessary spaces before "=" sign.

>+    } else {
>+      if (arguments.length > 2) {
>+        args = Array.prototype.slice.call(arguments);
>+        args.shift();
>+        args.shift();//Remove two args

See above about comment whitespace. This should be fixed throughout.

>+        if (aOptions.args && aOptions.args.length > 0) {
>+          args = Array.concat(aOptions.args, args);
>+        }
>+      } else if(aOptions.args && aOptions.args.length > 0) {

Space between "if" and "(". This should be fixed throughout.

>+        args = aOptions.args;
>+      } else {
>+        args = null;
>+      }
>+    }
>+    //2. Guard
>+    Guard.hasApply(aTask);

All the uses of Guard should go away since that's been r-'ed.

>+    //3. Proceed
>+    return Schedule._soon((aOptions && aOptions.delay) || 0,
>+                          (aOptions && aOptions.category) || null,
>+                          aTask,
>+                          args);
>+  },

Put a blank line between methods of the prototype.

>+  //As above, but without optional arguments
>+  _soon: function Schedule_soon_implem(aDelay, aCategory, aTask, aArgs) {
>+    let promise = new Promise();
>+    let closure = function Schedule_soon_closure() {
>+      try {
>+        let result = aTask.apply(aTask, aArgs);
>+        promise.resolve(result);
>+      } catch (x) {
>+        promise.reject(x);
>+      }
>+    };
>+    Schedule.execute._soon(aDelay, aCategory, promise, null, closure);
>+    return promise;
>+  },
>+  /**
>+   * Return an already resolved promise.
>+   *
>+   * This function is provided for convenience, to simplify combining
>+   * code that requires complex asynchronous computations and code that
>+   * only returns trivial results.
>+   *
>+   * @param {*} aValue The result of the promise.
>+   * @return {Promise} A promise ready to deliver |aValue|.
>+   */
>+  alreadyComplete: function(aValue) {
>+    let promise = new Promise();
>+    promise.resolve(aValue);
>+    return promise;
>+  },
>+  /**
>+   * As |soon| but without any wait.

s/As/Like/

>+   *
>+   * |Schedule.executeImmediately| is effectively equivalent to executing the
>+   * task immediately and wrapping its results or errors in an |Promise| object.
>+   * This function is provided as a utility to ease refactoring and debugging.
>+   *
>+   * This function does not even attempt to play nicely with other tasks.
>+   * Generally, you should not use it in new code. Future versions of this
>+   * library may print warnings if this function is used.
>+   *
>+   * @param {Function|*} aTask Either a function or an object
>+   * with a method |apply|.
>+   * @param {...*} aArguments Arguments to pass to |aTask|.
>+   * @return {Promise} An object that may be used to watch the execution
>+   * of the task and obtain its result.
>+   */
>+  executeImmediately:
>+    function Schedule_executeImmediately(aOptions, aTask /*...*/) {
>+      let args = Array.prototype.slice.call(arguments);
>+
>+      //1. Handle optional argument |aOptions|
>+      if (aOptions && aOptions.apply) {
>+        aTask = aOptions;
>+        args.shift();
>+      } else {
>+        args.shift();
>+        args.shift();
>+        if (aOptions.args && aOptions.args.length > 0) {

This logic looks suspicious. All non-zero numbers are truthy. Did you want:

    (typeof aOptions.args == 'number' && aOptions.args.length > 0) {

or

    (typeof aOptions.args != 'undefined' && aOptions.args.length > 0)

instead?

>+          args = Array.concat(aOptions.args, args);
>+        }
>+      }

That's kind of a gross way of destructuring the arguments vector. Would it not work to use array destructuring?

Also, I don't really understand what it's doing. It doesn't seem to match the doc comment, which doesn't say anything about an |args| option.

>+
>+      //2. Guard
>+      Guard.hasApply(aTask);
>+
>+      //3. Execute
>+      let promise = new Promise();
>+      let closure = function Schedule_executeImmediately_closure() {
>+        try {
>+          let result = aTask.apply(aTask, args);
>+          promise.resolve(result);
>+        } catch (x) {
>+          promise.reject(x);
>+        }
>+      };
>+      Schedule.execute._immediately(closure, closure, args);
>+      return promise;
>+    },
>+
>+  /**
>+   * A variant of |soon|, more limited, but which executes its contents
>+   * in another worker thread.
>+   *
>+   * @param {{delay:number|undefined,
>+   *         category:Schedule.Category|undefined,
>+   *         args:Array|undefined}=} aOptions A set of options, as an object
>+   *   which may have fields "delay" (a number of milliseconds to wait before
>+   *   executing), "category" (an optional category for the action - the
>+   *   scheduler will enforce ordering between actions of the same category
>+   *   and ensure that two actions of the same category are not executed
>+   *   concurrently), "args" (a set of args to pass to the task).
>+   * @param {Function|*} aTask Either a function or an object
>+   * with a method |apply|.
>+   * @param {...*} aArguments Arguments to pass to |aTask|.
>+   * @return {Promise} An object that may be used to watch the execution
>+   * of the task and obtain its result.
>+   */
>+  worker:        null /*FIXME: not implemented yet*/,

Never land code with a FIXME. If there's something that needs to be done, file it as a bug and put a comment with a reference to the bug in it.

I'm not sure about this interface. Workers don't take lambdas, they take source code to execute in a fresh context with no shared state. You *can* technically take a function and extract its source code with f.toString() but that's a really bad idea. :)

>+  /**
>+   * Obtain an |Promise| that can be used either for waiting upon completion of
>+   * a task or to set the task as resolved or rejected.
>+   *
>+   * Example:
>+   *   Schedule.on("ui-loaded").soon(alert, "It seems that UI is now loaded");
>+   *   Schedule.on("ui-loaded").resolve("I inform you that UI is now loaded");
>+   *
>+   * @return {Promise}
>+   */
>+  on: function Schedule_on(aName) {
>+    let promise = Schedule._observers[aName];
>+    if (!promise) {
>+      promise = new Promise();
>+      Schedule._observers[aName] = promise;
>+    }
>+    return promise;
>+  },
>+  _observers: [],
>+
>+  /**
>+   * Schedule a task without observing its result.
>+   *
>+   * These functions are provided for code who does not need Promises
>+   * or who handles Promises itself. Unless you are writing such code,
>+   * you should use |Schedule.soon|.
>+   */
>+  execute: {
>+    /**
>+     * Execute a function immediately.
>+     *
>+     * @param {*=} aContext A context to pass as |this|.
>+     * @param {Function} aFun  A function.
>+     * @param {Array} aArgs An array of arguments to pass to |aFun|
>+     */
>+    executeImmediately:
>+      function Schedule_execute_executeImmediately(aContext, aFun, aArgs) {
>+        if (!aFun || !aFun.apply) {//Handle optional argument
>+          aArgs = aFun;
>+          aFun  = aContext;
>+        }
>+        Guard.hasApply(aFun);
>+      Schedule.execute._immediately(aContext, aFun, aArgs);
>+    },
>+    //As above, but without argument checking
>+    _immediately:
>+      function Schedule_execute_executeImmediately_implem(aContext,
>+                                                          aFun, aArgs) {
>+      aFun.apply(aContext, aArgs);
>+    },
>+    /**
>+     * Execute a function "soon", as per |setTimeout|.
>+     *
>+     * @param {number|null} aDelay A number of milliseconds to wait.
>+     * @param {Schedule.Category|null} aCategory
>+     * @param {*=} aContext A context to pass as |this|.
>+     * @param {Function} aFun  A function to execute.
>+     * @param {Array} aArgs An array of arguments to pass to |aFun|
>+     */
>+    soon: function Schedule_execute_soon(aOptions, aFun) {
>+      let delay;
>+      let category;
>+      let promise;
>+      let context;
>+      let fun;
>+      let args;

Just put these in one let declaration separated by commas.

>+      //1. Handle optional argument |aOptions|
>+      if (aOptions.apply) {
>+        fun = aOptions;
>+        delay = 0;
>+        category = null;
>+        promise = null;
>+        context = null;
>+        args = null;
>+      } else {
>+        if (!aOptions) {
>+          aOptions = {};//FIXME: Any arbitrary object would do
>+        }
>+        fun = aFun;
>+        delay = aOptions.delay || null;
>+        category = aOptions.category || null;
>+        promise = aOptions.promise || null;
>+        if (category && !promise) {
>+          throw new Error("Schedule.execute.soon: called with a category"
>+                          +"but no promise");

Space between "+" and the string literal.

>+        }
>+        context = aOptions.context || null;
>+        args = aOption.args || null;
>+      }
>+      Guard.hasApply(fun);
>+      Schedule.execute._soon(delay, category, promise, context, fun, args);
>+    },
>+    //As above, but without optional arguments
>+    _soon: function Schedule_execute_soon_implem(aDelay, aCategory, aPromise,
>+                                                 aContext, aFun, aArgs) {
>+      let closure = function Schedule_execute_soon_closure() {
>+        if (!aCategory) {
>+          aFun.apply(aContext, aArgs);
>+        } else {
>+          aCategory.push(
>+            function() {
>+              aFun.apply(aContext, aArgs);
>+            },
>+            aPromise
>+          );
>+        }
>+      };
>+      Schedule.execute._soonClosure(aDelay, closure);
>+    }
>+  },
>+
>+  /**
>+   * A category of tasks.
>+   *
>+   * Tasks which rely on execution order or should avoid being interleaved
>+   * for performance reasons (e.g. disk access) should be registered as
>+   * with a category to enforce execution order.
>+   *
>+   * @constructor
>+   *
>+   * @param {string} aName A name, used for logging/debugging purposes.
>+   */
>+  Category: function(aName) {
>+    this._queue = [];
>+    this._executionInProgress = false;
>+    this._name = aName;
>+  }
>+};
>+function dumpn(aText) {
>+  dump(aText+"\n");
>+}

Don't leave debugging code in.

>+Schedule.Category.prototype = {
>+  /**
>+   * Add a new task to this Category.
>+   *
>+   * If the Category is currently executing a task, this task will be
>+   * executed later. Otherwise, the task is executed immediately.
>+   *
>+   * In a Category, tasks are always executed in the order in which they
>+   * are received.
>+   *
>+   * @param {Function} aTask A function or an object with method |call|.
>+   * @param {Promise} aPromise A promise attached to |aTask| and which
>+   * will be informed when |aTask| is complete.
>+   */
>+  push: function Schedule_Category_push(aTask, aPromise) {
>+    Guard.isNotNull(aTask);
>+    Guard.isNotNull(aPromise);
>+    let pop = this._pop.bind(this);
>+    aPromise.then(pop, pop);
>+    if (!this._executionInProgress) {
>+      this._executionInProgress = true;
>+      Schedule.execute._soonClosure(0, aTask);
>+    } else {
>+      this._queue.push(aTask);
>+    }
>+  },
>+
>+  /**
>+   * Mark completion of a task, possibly triggering some task
>+   * that has been waiting.
>+   */
>+  _pop: function Schedule_Category_pop() {
>+    let next = this._queue.pop();
>+    if (next) {
>+      Schedule.execute._soonClosure(0, next);
>+    } else {
>+      this._executionInProgress = false;
>+    }
>+  }
>+};
>+
>+/**
>+ * Enqueue a tasks after this promise.
>+ *
>+ * This method behaves as |makeChain| but enforces asynchronous execution
>+ * of the resulting tasks.
>+ *
>+ * @param aOnSuccess A function to call in case of success.
>+ * @return {Promise} A new promise that observes the success/error/failure
>+ * of both |this| and |aOnSuccess|.
>+ */

OK, this was the part that threw me the most. It took me a long time to understand the |Schedule| examples at the top of the file, because I couldn't figure out how promises had a |soon| method. Nothing in Promise.jsm said anything about it. It wasn't until I got to this "Internal stuff" part of the file that I realized you were monkey-patching Promise!

I understand the motivation, and I agree that having the promises returned by Schedule methods be chainable to subsequent scheduler events is a nice way of getting left-to-right composition. But I don't like the monkey-patching. (And it's not good that you don't document this anywhere). It's "action at a distance," a coupling of the Schedule and Promise API's. My first thought was, if you're going to couple the two API's, just put the scheduling methods into the Promise API directly in Promise.jsm. But that would mean the two modules are mutually recursive -- I'm not even sure if you can do that with jsm's -- and you really don't want the two-way coupling. It should be possible to use the Promise API without the Schedule API leaking into it at all.

I think this is actually a good use case for inheritance. I suggest a SchedulePromise class:

    function SchedulePromise() {
        Promise.call(this);
    }
    SchedulePromise.prototype = new Promise();
    SchedulePromise.prototype.soon = ...;
    SchedulePromise.prototype.soonWithArgs = ...;
    SchedulePromise.prototype.executeImmediately = ...;

And then all the places where the Scheduler object currently returns Promise instances, it should now return SchedulePromise instances.

You might be able to think of a prettier name than SchedulePromise. :)

>diff --git a/toolkit/content/tests/unit/test_schedule.js b/toolkit/content/tests/unit/test_schedule.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/content/tests/unit/test_schedule.js
>@@ -0,0 +1,111 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is JSFileAPI.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *    David Rajchenbach-Teller <[e-mail]> (Original author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+Components.utils.import("resource://gre/modules/schedule.jsm");
>+Components.utils.import("resource:///modules/devtools/Promise.jsm");
>+
>+
>+function checkpoint(info) {
>+  return function(arg) {
>+    dump("checkpoint: "+info+", "+arg+"\n");
>+  };
>+}
>+
>+function run_test() {
>+  do_test_pending();
>+  let tasks = [];
>+  let on = Schedule.on;
>+  let cat = new Schedule.Category("XPCTEST");
>+
>+  let counter = 0;
>+  function add(aTask) {
>+    tasks.push(aTask);
>+    aTask.count = ++counter;
>+  }
>+  add(on("ready").chainPromise(checkpoint("A")));
>+  add(on("ready").soon(checkpoint("B")));
>+  add(on("ready").soon({delay: 10}, checkpoint("C")));
>+  add(on("ready").soon({delay: 100}, checkpoint("D")));
>+  add(on("ready").executeImmediately(checkpoint("E")));
>+
>+
>+  add(Schedule.soon(checkpoint("B2")));
>+  add(Schedule.soon({delay: 10}, checkpoint("C2")));
>+  add(Schedule.soon({delay: 100}, checkpoint("D2")));
>+  add(Schedule.executeImmediately(checkpoint("E2")));
>+
>+  add(Schedule.soon({category: cat}, checkpoint("B3")));
>+  add(Schedule.soon({delay: 10, category: cat}, checkpoint("C3")));
>+  add(Schedule.soon({delay: 100, category: cat}, checkpoint("D3")));
>+
>+  var group = Promise.group(tasks);
>+  group.then(
>+    function(){
>+      do_test_finished();
>+    },
>+    function(error) {
>+      dump(error);
>+      dump(error.stack);
>+      do_throw();
>+    }
>+  );
>+
>+  function taskToString(aTask) {
>+    return ""
>+      +aTask.count
>+      +", "
>+      +aTask.isComplete()
>+      +", "
>+      +aTask.isResolved()
>+      +", "
>+      +aTask.isRejected();

Space after the "+" signs.

Overall it looks good, but I'd like to see it one more time before a final r+. I'll be fast with the next review.

Dave

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Attachment: v5. Module Schedule

Comment 34

[David Teller [:Yoric] - still alive but not very active]

(In reply to Dave Herman [:dherman] from comment #32)
> Comment on attachment 581599 [details] [diff] [review]
> v4. Module Schedule
> 
> >+ * - schedule immediately a bunch of code for execution with a call such as
> >+ *   Schedule.soon(function() { ... })
> >+ *
> >+ * - add a delay before execution
> >+ *   Schedule.soon({delay: 100}, function() { ... })
> >+ *
> >+ * - group tasks by categories to ensure execution order and avoid simultanous/
> >+ *   conflicting use of resources:
> >+ *   Schedule.soon({category: DISK}, function() { ... })
> 
> I think you got rid of DISK so you should get rid of this example. Did you
> intend to get rid of categories altogether for now, too?

Actually, only the DISK category. Categories are very much implemented, but DISK is now in the corresponding module.

> Also mention the ability to pass arguments for the thunk in the Schedule
> JavaDoc comment too?

Done, thanks.

> 
> I don't understand the naming convention (the "a" prefix). Is this a Mozilla
> convention I'm not familiar with?

It is listed in our Coding Guidelines (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Prefixes). As JavaScript requires |this.| to dereference fields/methods, this is not required, so this guideline might be intended only for C++, I am not sure. For the moment, I have adopted it in JS.

> All the uses of Guard should go away since that's been r-'ed.

Done, with a heavy heart.

> Put a blank line between methods of the prototype.

Done.

> s/As/Like/

Actually, found out that you can already do it with a one-liner. Removed the function.

> >+      //1. Handle optional argument |aOptions|
> >+      if (aOptions && aOptions.apply) {
> >+        aTask = aOptions;
> >+        args.shift();
> >+      } else {
> >+        args.shift();
> >+        args.shift();
> >+        if (aOptions.args && aOptions.args.length > 0) {
> 
> This logic looks suspicious. All non-zero numbers are truthy. Did you want:
> 
>     (typeof aOptions.args == 'number' && aOptions.args.length > 0) {
> 
> or
> 
>     (typeof aOptions.args != 'undefined' && aOptions.args.length > 0)
> 
> instead?

Er, no. Actually, |aOptions.args| is an array-like field. I do not know of any method for checking that an object is an array-like. Here, I am counting on an error either when dereferencing |aOptions.args.length| or at the |Array.concat| of the next line if the object is not array-like.

> 
> >+          args = Array.concat(aOptions.args, args);
> >+        }
> >+      }
> 
> That's kind of a gross way of destructuring the arguments vector. Would it
> not work to use array destructuring?
> 
> Also, I don't really understand what it's doing. It doesn't seem to match
> the doc comment, which doesn't say anything about an |args| option.

I fully agree that it is gross. If you know a way of getting the cddr of an array in JavaScript, I am very interested.

> >+  worker:        null /*FIXME: not implemented yet*/,
> 
> Never land code with a FIXME. If there's something that needs to be done,
> file it as a bug and put a comment with a reference to the bug in it.

Sure. For the moment, however, I have not reached the stage at which I wish to land that code – although we are getting there. I merely followed the guidelines of my team, which state that we should request reviews early and often.

> I'm not sure about this interface. Workers don't take lambdas, they take
> source code to execute in a fresh context with no shared state. You *can*
> technically take a function and extract its source code with f.toString()
> but that's a really bad idea. :)

I am very aware of the problem. I have been toying with an alternative API, but it is not ready yet, even conceptually. For the moment, I will just remove |worker| from the API.

> Just put these in one let declaration separated by commas.
Ok.

> Space between "+" and the string literal.
Done.

> Don't leave debugging code in.
This debugging code is actually quite useful to debug API clients. I have not found any good way of doing this. Would it be acceptable if made my definition of |dumpn| conditional upon a |if (gDebug) { ... }|?

> I think this is actually a good use case for inheritance. I suggest a
> SchedulePromise class:

And me who thought that monkey-patching was the whole point of using a dynamic language :)

More seriously, done.

> Space after the "+" signs.

Done.

Thanks for the review,
 David

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Attachment: v5. Module Schedule

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Attachment: v6. Module Schedule

Updated version, with the following changes, as per various feedback:
- |soon| has been renamed |queue|;
- instead of patching |Promise|, we now have a class called |Schedule|, which inherits from |Promise| and adds method |queue|;
- |queue| now accepts a new option |cutQueue| that lets a task be added at the head of a category rather than pushed at the end;
- additional clean-up, removing some dead code.

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Review ping?

Comment 38

[Dave Herman [:dherman]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #34)

> > >+        if (aOptions.args && aOptions.args.length > 0) {
> > 
> > This logic looks suspicious. All non-zero numbers are truthy. Did you want:
> > 
> >     (typeof aOptions.args == 'number' && aOptions.args.length > 0) {
> > 
> > or
> > 
> >     (typeof aOptions.args != 'undefined' && aOptions.args.length > 0)
> > 
> > instead?
> 
> Er, no. Actually, |aOptions.args| is an array-like field.

Oh, I'm dumb, I didn't notice that you're testing aOptions.args for truthiness instead of aOptions.args.length, and even managed to copy and paste it into my suggestions without noticing. Just ignore me.

> > >+          args = Array.concat(aOptions.args, args);
> > >+        }
> > >+      }
> > 
> > That's kind of a gross way of destructuring the arguments vector. Would it
> > not work to use array destructuring?
> > 
> > Also, I don't really understand what it's doing. It doesn't seem to match
> > the doc comment, which doesn't say anything about an |args| option.
> 
> I fully agree that it is gross. If you know a way of getting the cddr of an
> array in JavaScript, I am very interested.

Oh, I see. Yeah, destructuring doesn't currently have a convenient way of doing that. But the uses of shift() are unnecessary; just replace it with:

    args = Array.prototype.slice.call(arguments, 2);

> > >+  worker:        null /*FIXME: not implemented yet*/,
> > 
> > Never land code with a FIXME. If there's something that needs to be done,
> > file it as a bug and put a comment with a reference to the bug in it.
> 
> Sure. For the moment, however, I have not reached the stage at which I wish
> to land that code – although we are getting there. I merely followed the
> guidelines of my team, which state that we should request reviews early and
> often.

Huh. Well, normally if you want feedback for something that's not ready to land, you ask for feedback instead of review. I can't r+ with a FIXME.

> > Don't leave debugging code in.
> This debugging code is actually quite useful to debug API clients. I have
> not found any good way of doing this. Would it be acceptable if made my
> definition of |dumpn| conditional upon a |if (gDebug) { ... }|?

I don't know, that seems worse to me. I guess I'm not clear on what our policy is about debugging functions in JS. I won't stand in the way.

> > I think this is actually a good use case for inheritance. I suggest a
> > SchedulePromise class:
> 
> And me who thought that monkey-patching was the whole point of using a
> dynamic language :)

I hope you really are kidding...

Updated review forthcoming.

Dave

Comment 39

[Dave Herman [:dherman]]

Comment on attachment 586439 [details] [diff] [review]
v6. Module Schedule

># HG changeset patch
># Parent dafde2d7d8b366255ff5644511805f0d577690f6
>
>diff --git a/toolkit/content/Makefile.in b/toolkit/content/Makefile.in
>--- a/toolkit/content/Makefile.in
>+++ b/toolkit/content/Makefile.in
>@@ -86,16 +86,17 @@ DIRS += tests
> endif
> 
> EXTRA_JS_MODULES = \
>   Geometry.jsm \
>   InlineSpellChecker.jsm \
>   PopupNotifications.jsm \
>   Dict.jsm \
>   guards.jsm \

Looks like you haven't removed guards.jsm from your source tree. I would expect this to break the build.

>+/******************************************
>+ * Object |Schedule|
>+ *
>+ * A mechanism for launching tasks asynchronously and chaining asynchronous
>+ * tasks.
>+ *
>+ *
>+ * Examples:
>+ *
>+ * - schedule immediately a bunch of code for execution with a call such as
>+ *   Schedule.queue(function() { ... })
>+ *
>+ * - add a delay before execution
>+ *   Schedule.queue({delay: 100}, function() { ... })
>+ *
>+ * - group tasks by categories to ensure execution order and avoid simultanous/

simultaneous

>+  queue: function Schedule_queue(aOptions, aTask) {
>+    let args;
>+    // 1. Handle optional argument |aOptions|
>+    if (typeof aOptions == "function" || typeof aOptions.apply == "function") {
>+      args = Array.prototype.slice.call(arguments);
>+      args.shift(); // Remove one arg

Replace these two lines with:

args = Array.prototype.slice.call(arguments, 1);

>+      aTask = aOptions;
>+    } else {
>+      if (arguments.length > 2) {
>+        args = Array.prototype.slice.call(arguments);
>+        args.shift();
>+        args.shift(); // Remove two args

Replace these there lines with:

args = Array.prototype.slice.call(arguments, 2);


>+  // As above, but without optional arguments
>+  _queue: function Schedule_queue_implem(aDelay, aCategory, aContext,
>+                                         aTask, aArgs) {
>+    let future = new Future();
>+    let resolve = function(aResolution) {
>+      if (aResolution instanceof Future) {
>+        aResolution.then(resolve, reject);

I've noticed other promise libraries doing this kind of thing too and it makes me nervous. IIUC, you're saying that a computation that resolves to a Future should not actually be considered resolved but should instead continue waiting. Do I have that right? If so, it seems fragile, and not fully polymorphic (for example, if you resolve with a value provided by someone else, you might not know whether they happen to be producing a Future). And it's not necessary, since a client who wants to do a computation in several steps can still chain their promises anyway.

In the context of web content development, I also generally look instanceof with suspicion, since it breaks down in the presence of multiple global objects. But that's probably irrelevant here, since this is in the single shared global context of front-end code.

+/**
+ * A task that has been scheduled for execution.
+ *
+ * This constructor returns |Promise|s that can interact
+ * with the scheduler through a method |queue|.
+ *
+ * @constructor
+ * @extends {Promise}
+ */
+function Future()
+{
+  Promise.call(this);
+}

Nice. This seems like a reasonable choice of terminology.

>+/**
>+ * Construct a |Future| from an existing |Promise|.
>+ *
>+ * @return {Future}
>+ */
>+Future.ofPromise = function Future_ofPromise(aPromise)

I'd call it Future.fromPromise.

>+{
>+  // 1. Guard
>+  if (! (aPromise instanceof Promise)) {
>+    throw new TypeError("|Future.ofPromise| expects a |Promise|");
>+  }

This is another place where I generally dislike the use of instanceof; I prefer to think of "promise" as an abstract structural type, where any implementation would do. But if we really are just going to use the one single promise library everywhere, I suppose this is okay. I still think you're being over-conservative with your type testing. But again, I won't get in the way.

r=me with the above nits addressed; I leave it up to you to decide what you think about the couple semantic questions I raised.

Dave

Comment 40

[David Teller [:Yoric] - still alive but not very active]

(In reply to Dave Herman [:dherman] from comment #39)
> 
> Looks like you haven't removed guards.jsm from your source tree. I would
> expect this to break the build.

My bad.

> Replace these two lines with:
> 
> args = Array.prototype.slice.call(arguments, 1);

Ah, of course.

> I've noticed other promise libraries doing this kind of thing too and it
> makes me nervous. IIUC, you're saying that a computation that resolves to a
> Future should not actually be considered resolved but should instead
> continue waiting. Do I have that right? If so, it seems fragile, and not
> fully polymorphic (for example, if you resolve with a value provided by
> someone else, you might not know whether they happen to be producing a
> Future). And it's not necessary, since a client who wants to do a
> computation in several steps can still chain their promises anyway.

I understand the issue. My first version of the scheduler was well-typed and did no such collapsing, until I realized that this made the examples of |OS.File| much harder to explain. So far, I have had no reason to complain from this collapsing, as it makes code easier to read and to debug. I grant you that this will very suddenly stop being the case if someone actually wants to build a library that promises promises, but I am not sure that we want to encourage this either.

> In the context of web content development, I also generally look instanceof
> with suspicion, since it breaks down in the presence of multiple global
> objects. But that's probably irrelevant here, since this is in the single
> shared global context of front-end code.

In this case, I cannot think of anything that would break. Unless we suddenly and drastically change the semantics of modules, which would be a shame.

> +function Future()
> +{
> +  Promise.call(this);
> +}
> 
> Nice. This seems like a reasonable choice of terminology.

Actually, I have just reworked it. In the latest version of the patch, the object is called |Schedule|, not |Future|.

> >+Future.ofPromise = function Future_ofPromise(aPromise)
> 
> I'd call it Future.fromPromise.

Ok.

> 
> >+{
> >+  // 1. Guard
> >+  if (! (aPromise instanceof Promise)) {
> >+    throw new TypeError("|Future.ofPromise| expects a |Promise|");
> >+  }
> 
> This is another place where I generally dislike the use of instanceof; I
> prefer to think of "promise" as an abstract structural type, where any
> implementation would do. But if we really are just going to use the one
> single promise library everywhere, I suppose this is okay. I still think
> you're being over-conservative with your type testing. But again, I won't
> get in the way.

In code as delicate and hard-to-debug as asynchronous programming, I prefer to err on the side of overzealousness.

> r=me with the above nits addressed; I leave it up to you to decide what you
> think about the couple semantic questions I raised.
> 
> Dave

Thanks for the review. I attach a new version.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Attachment: Module Schedule

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Attachment: Module Schedule

Oops, I had left "debug = true".

Comment 43

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

The companion testsuite.

Comment 44

[Dave Herman [:dherman]]

Comment on attachment 594212 [details] [diff] [review]
Module Schedule

>+/******************************************
>+ * Object |Schedule|
>+ *
>+ * A mechanism for launching tasks asynchronously and chaining asynchronous
>+ * tasks.

It's fine to use this as the name of what was called Future in the previous iteration, but also add to the doc comments what it means to create a new Schedule(), i.e., the doc comment you had on Future in the previous version.

>+/**
>+ * Place the scheduler in debug mode.
>+ *
>+ * If set to |true|, creating a new |Schedule| will capture additional
>+ * information to aid in debugging. In the current version, this information
>+ * consists in the stack that leads to the creation of the |Schedule|. This
>+ * information is displayed in case of uncaught exception.
>+ *
>+ * Note that this can be quite expensive in terms of memory usage and speed.
>+ *
>+ * |false| by default.
>+ */
>+Schedule.__defineSetter__(
>+  "DEBUG",
>+  function(aValue) {
>+    Schedule._DEBUG = aValue;
>+  }
>+);
>+Schedule._DEBUG = false;

Is this setter/pseudo-private pair of privates doing anything different from simply providing a public DEBUG property? It seems pointless. But if you do use a setter, it's better to use ES5 standard mechanisms:

    Object.defineProperty(Schedule, "DEBUG", { get: function() { ... }, set: function(aValue) { ... }, enumerable: true, configurable: true });

Looks good. r=me with above minor issues addressed.

Dave

Comment 45

[Dave Herman [:dherman]]

Comment on attachment 594214 [details] [diff] [review]
Companion testsuite

r=me

Comment 46

[David Teller [:Yoric] - still alive but not very active]

(In reply to Dave Herman [:dherman] from comment #44)
> It's fine to use this as the name of what was called Future in the previous
> iteration, but also add to the doc comments what it means to create a new
> Schedule(), i.e., the doc comment you had on Future in the previous version.

Good point.

> 
> >+/**
> >+ * Place the scheduler in debug mode.

You are right on both accounts. For the moment, I will remove the setter, it is not really useful.

> Looks good. r=me with above minor issues addressed.

Thanks!

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Attachment: Module Schedule

Comment 48

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion Testsuite

Comment 49

[David Teller [:Yoric] - still alive but not very active]

Ready to land, once blocking bugs have landed.

Comment 51

[David Teller [:Yoric] - still alive but not very active]

Attachment: Module Schedule

Comment 52

[David Teller [:Yoric] - still alive but not very active]

Attachment: Module Schedule

Comment 53

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 54

[David Teller [:Yoric] - still alive but not very active]

Attachment: Module Schedule

After a conversation with Gavin, I decided to get back to work on this patch and simplify the code. Let me present a well-shrunk patch. We have lost two (undocumented) features: |executeImmediately| and |cutQueue|, neither of which were attached to precise scenarios yet. The code also does not attempt to work nicely from a worker thread yet. This will be the topic of a followup bug. In exchange, the code is more readable and has lost 25% of its size.

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 56

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 57

[David Teller [:Yoric] - still alive but not very active]

Attachment: Module Schedule

I have just decided to remove scheduling categories. For the moment, I have not seen any clear use case for this feature, and I suspect that it can be implemented differently and more orthogonally.

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 608715 [details] [diff] [review]
Module Schedule

Removing this from review by Gavin, as it is not nearly as urgent as some other patches.

Comment 59

[Dave Herman [:dherman]]

What's the status of this bug? I've gotten so many r?s on it, I'd rather not spend any more time doing a review unless it's really the final review.

Thanks,
Dave

Comment 60

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 608715 [details] [diff] [review]
Module Schedule

Ah, right, I had almost forgotten this bug. This implementation is now deprecated. All dependent code has been rewritten to not use promises. We might resurrect this some day, with a different implementation.

Comment 61

[Richard Newman [:rnewman]]

Yoric: would you please do one of the following?

* Close this bug as WONTFIX or INCOMPLETE, if you're confident in that
* Dupe it to another bug that does the same thing, and fix dependencies
* Unassign it, mark your patches as obsolete, and clarify that a new implementation is welcome to take over this bug?

Right now it's a bit up in the air, but the need seems to still exist.

Thanks!

Comment 62

[David Teller [:Yoric] - still alive but not very active]

Attachment: Lightweight implementation of barriers

Let's give this another try. Attaching a draft implementation of a system of global barriers.

Comment 63

[Gregory Szorc [:gps]]

Comment on attachment 700293 [details] [diff] [review]
Lightweight implementation of barriers

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

You've created a default-on-missing-on-read Map that returns promises. If you think this is a generally useful thing to have in the tree, sure.

I like the idea of using barriers to gate startup tasks/services. But, the implementation of said system will likely involve strong coupling with the startup service. We probably want things like priorities, manifest registration of dependencies, etc.

Comment 64

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 700293 [details] [diff] [review]
Lightweight implementation of barriers

Indeed it is hard to comment on the general utility of something like this in the abstract. Are there initial users for this module in mind?

Comment 65

[Richard Newman [:rnewman]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #64)

> Indeed it is hard to comment on the general utility of something like this
> in the abstract. Are there initial users for this module in mind?

One is FHR, which initializes after ten seconds.

Comment 66

[David Teller [:Yoric] - still alive but not very active]

Not working actively on this.

Comment 67

[David Teller [:Yoric] - still alive but not very active]

Let's reopen this bug when we have a clearer scenario.