listTree performance about q-io HOT 12 OPEN

listTree has a conceptual problem since it produces a promise for an array. My hope is to replace that with a promise for a Q stream in a future release—perhaps under a different name to retain backward compatibility—or a promise for completion and just use the guard emitter for streaming purposes.

from q-io.

That sounds pretty cool @ Q streams. In a particular case that I am working on, I need stat results on all files to check their mtimes, and so it is no problem if I get the entire array all at once; (The only problem is that after I listTree(), then I have to run stat() on all the paths. It sounds like Q streams or the guard emitter might help with that... but that's a separate issue from this.)

Edit: I should say: streaming isn't a problem yet here for me, and probably won't be in this application. My directory trees and associated data aren't big enough to eat up all the RAM - they can happily be all resident in memory for now. But I can see benefit to streaming though otherwise.

from q-io.

The guard function can already do what you need, I think.

FS.listTree(path, function (name, stat) {
    // return true to include
    // return false to exclude
    // return null to exclude and skip subtree
}))
.then(function () {
})

from q-io.

That's a cool trick with guard. Looks like this would do the trick of snarfing the stats:

var stats = [];
FS.listTree(path, function(name, stat) {
  stats.push([name, stat])
  return true; // edit: don't forget, this is a guard function afterall
})
.then(function() {
  return stats;
})
.then(function(stats) {
  // awesome, got the stats I need!
});

But this doesn't address the speed gap of using Q.

The issue here is how long it takes listTree() to finish. The fact that it collects a list isn't really a problem. Using Q on stat and readdir inside of listTree is the primary cause of the slow down. I can rewrite listTree() to internally not use promises, and then it works like a little speed demon. Obviously, then it loses the clarity that promises provide which is undesirable. So, I'm wondering if there is potential to identify a specific bottleneck that for some reason causes Q-code to be slower than Node.js callback-style code (and by a large factor: 10x).

from q-io.

@dhbaird I presume that the problem is recursive array aggregation. It might be faster to pass a single array forward and push results onto it instead of concatenating results from all. Of course, listTree may lose some of the clarity in the process.

from q-io.

I thought recursive array aggregation might have been a problem too, but take a look at asyncFindTree(). It uses recursive array aggregation (see its nested checkCompletion() function). And it's fast.

When you get a moment, could you take a look at the following: asyncFindTree() and asyncFindTreeQstat(). The difference in code is very small: a single Q.nfcall() is added, and that results in a substantial performance deviation. The run time goes from 3 seconds to 18 seconds.

Edit: Here's the diff (to make it easier to compare the two):

--- a.js        2013-08-12 17:58:04.111106000 -0600
+++ b.js        2013-08-12 17:58:07.262917000 -0600
@@ -1,7 +1,9 @@
+var Q = require('q');
 var path = require('path');
 var FS = require('fs');
-function asyncFindTree(basePath, cb) {
-  FS.stat(basePath, function(error, stat) {
+function asyncFindTreeQstat(basePath, cb) {
+  return Q.nfcall(FS.stat, basePath)
+  .then(function(stat) {
     if (stat.isDirectory()) {
       FS.readdir(basePath, function(error, children) {
         children = children.map(function(child) { return path.join(basePath, child); });

from q-io.

(Note - I edited previous comment to include diff)

from q-io.

@dhbaird That is very curious. nfcall does do some extra work in this case, coercing FS.stat to a promise for FS.stat, manipulating the arguments, sending the apply message to that promise, but I would not expect it to dominate the performance like that. You might try using Q.nfbind instead of Q.nfcall to cache the node-ified function in the parent scope to see whether that makes a difference.

from q-io.

@kriskowal something like this?

var stat2 = Q.nfbind(FS.stat); // aka denodeify
function asyncFindTreeQstat(basePath, cb) {
  return stat2(basePath)
  .then(function(stat) {
...

Good idea, but performance remains about the same.

from q-io.

Weirder and weirder.

I am going to have to let this issue idle for a while, but if you send a PR that helps the issue without impacting the API, I can get a patch out quickly. If it requires an API change, I can entertain a proposal as well.

from q-io.

Sounds good to me. I just wanted to get the word out, and I'm happy if you want to close this and re-open it later. I'll keep looking into it.

from q-io.

I will probably introduce listStream() and listTreeStream() in the v2 branch.

from q-io.