Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to mark the top level #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add the ability to mark the top level #61

wants to merge 1 commit into from

Conversation

ForbesLindesay
Copy link
Collaborator

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.

@domenic
Copy link
Collaborator

domenic commented Jun 15, 2015

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

@ForbesLindesay
Copy link
Collaborator Author

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.

@domenic
Copy link
Collaborator

domenic commented Jun 15, 2015

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

@ForbesLindesay
Copy link
Collaborator Author

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.

@rkatic
Copy link
Collaborator

rkatic commented Jun 26, 2015

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

@rkatic
Copy link
Collaborator

rkatic commented Jun 28, 2015

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.

@ForbesLindesay
Copy link
Collaborator Author

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

@rkatic
Copy link
Collaborator

rkatic commented Jul 2, 2015

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
Copy link
Collaborator

rkatic commented Jul 2, 2015

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
Copy link
Collaborator

rkatic commented Jul 2, 2015

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?

@kriskowal
Copy link
Owner

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.

@rkatic
Copy link
Collaborator

rkatic commented Jul 3, 2015

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();
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants