asap icon indicating copy to clipboard operation
asap copied to clipboard

Add the ability to mark the top level

Open ForbesLindesay opened this issue 9 years ago • 12 comments

This is a request for comments. If people think it's worthwhile and agree with the proposed API, I'll add it to the other interfaces.

The idea is that you could do something like:

var fs = require('fs');
var asap = require('asap/raw');

var readFile = fs.readFile;
fs.readFile = function (filename, encoding, callback) {
  if (callback === undefined) {
    callback = encoding;
    encoding = undefined;
  }
  readFile(filename, encoding, function (err, res) {
    asap.markFlushing();
    callback.apply(this, arguments);
    asap.flush();
  });
};

This avoids an extra nextTick call when we know we are already at the top of the stack.

ForbesLindesay avatar Jun 15 '15 11:06 ForbesLindesay

I don't understand. Can you explain this in more detail?

domenic avatar Jun 15 '15 15:06 domenic

The goal of calling asap is to ensure the stack is cleared of all user code (i.e. next micro-tick). Any internal method that is asynchronous naturally clears the stack of all user code. e.g. fs.readFile on node.js. It would therefore be possible safe to rewrite the following:

console.log('a');
fs.readFile('foo.js', function () {
  console.log('c');
  asap(function () {
    console.log('e');
  });
  console.log('d');
});
console.log('b');

as:

console.log('a');
fs.readFile('foo.js', function () {
  console.log('c');
  console.log('d');
  function () {
    console.log('e');
  }();
});
console.log('b');

This effectively removes the need for asap to call a more expensive nextTick implementation such as process.nextTick or setImmediate.

ForbesLindesay avatar Jun 15 '15 15:06 ForbesLindesay

Sure. So why not just don't use asap in that case? I still don't understand the new API added.

domenic avatar Jun 15 '15 15:06 domenic

Because the two things are not necessarily related. i.e. my Promise library uses asap, but has no knowledge of fs.readFile. Someone could separately take the trouble to mark when things create fresh stack, and thereby make my promise library faster without my promise library needing to do the work itself of tracking all async operations.

ForbesLindesay avatar Jun 15 '15 15:06 ForbesLindesay

@ForbesLindesay Sorry if I'm missing something obvious, but can you please explain why this API would be better then an asap.flushSync?

rkatic avatar Jun 26 '15 21:06 rkatic

To be more clear, an asap.flushSync would do both: mark flushing, and do flushing. So it's not clear to me why it's necessary to have an API with "marking" and actual flushing separated.

rkatic avatar Jun 28 '15 10:06 rkatic

You need to mark before asap is called to avoid a flush being scheduled. You then need to trigger the actual flush afterwards.

ForbesLindesay avatar Jul 01 '15 14:07 ForbesLindesay

Or I'm still missing something obvious, or my question was not clear enough.

Consider

asap.flushSync = function() {
  flushing = true;
  flush();
};

Why your first example is better then

var fs = require('fs');
var asap = require('asap/raw');

var readFile = fs.readFile;
fs.readFile = function (filename, encoding, callback) {
  if (callback === undefined) {
    callback = encoding;
    encoding = undefined;
  }
  readFile(filename, encoding, function (err, res) {
    callback.apply(this, arguments);
    asap.flushSync();
  });
};

Can you please show a scenario where asap.flushSync will not suffice?

rkatic avatar Jul 02 '15 00:07 rkatic

On a second tough, I'm not sure marking is even necessary if we flush synchronously, since it's only used to avoid recursive process.nextTick. Hmm, I guess I am missing something obvious here...

rkatic avatar Jul 02 '15 00:07 rkatic

Ok, I see your point now. With asap.flushSync you would flush without waiting next tick, but process.nextTick would still be called during flushing. Not sure that an extra process.nextTick on each IO event would make any difference, but I can see the usefulness of your API specially when transferred to browsers. ~~Anyhow, I think we can do better on implementing this, and maybe in finding better names for those new functions.~~ Also I don't see why not to offer such API to browsers too. (We could implement the API and do some real tests to see if the new API performs better with or without "marking", and then decide to keep or remove asap.markFlushing.)

@ForbesLindesay are you willing to add similar changes to browser-raw.js too?

rkatic avatar Jul 02 '15 18:07 rkatic

Before we talk about API changes to support tail tasks, we need to addeess #56. At present, I would be embarrassed to promote anything based on ASAP, which for what it is worth, is everything I work on in my spare time these days. The debugging experience in Chrome is broken. Debugging is more important than speed, though we should strive to have both. That may entrain detecting whether the inspector is open (There is a package for that).

I am interested in tail tasks, on the condition that we do not consume unbounded stack frames. I have not given this PR a lot of attention. Please continue discussing and summarizing.

kriskowal avatar Jul 02 '15 18:07 kriskowal

I gave a little more thought on this, and realized that the two function API is probably not adequate. If user throws between marking and flushing, the synchronous flushing will not execute and will remain permanently remain in "flushing" state. Of course, to fix that, API user could wrap user callbacks in a try-finally block, but that would be horrible, and we can do much better.

We could define a asap.tailTask(task) like this:

asap.tailTask = function (task) {
  flushing = true; // TODO: avoid flushing in asap if flushing == true
  if (task) asap(task);
  flush();
};

Now the first example becomes:

var fs = require('fs');
var asap = require('asap/raw');

var readFile = fs.readFile;
fs.readFile = function (filename, encoding, callback) {
  if (callback === undefined) {
    callback = encoding;
    encoding = undefined;
  }
  readFile(filename, encoding, function (err, res) {
    asap.tailTask(callback.bind(this, err, res));
  });
};

EDIT: Now, since asap here should be the one from asap.js, we could use the two function API proposed from @ForbesLindesay to add following to asap.js:

asap.tailTask = fuction (task) {
  rawAsap.markFlushing();
  if (task) asap(task);
  rawAsap.flush();
};

rkatic avatar Jul 03 '15 05:07 rkatic