ybogdanov / node-sync

Write simple and readable synchronous code in nodejs using fibers
MIT License
492 stars 70 forks source link

Performance issue #29

Open weizhu opened 11 years ago

weizhu commented 11 years ago

Hi,

I've using node-sync for a while and love it. However, when my code starts to doing a large number of async call with Sync.future, I am running into serious performance issue. I narrowed it down to node-sync module.

Here is my test code that does't nothing but doing 2000 empty async full on AWS m1.small instance and it took 847ms.

At this level of performance, it's almost unusable. Help!

// Versions
// ├─┬ sync@0.2.2
//│ └── fibers@1.0.0

"use strict";
var Sync = require('sync');

var getByIdCb= function(id, cb) {
  cb(null, 'foo');
}

var getByIdAsync = function(id, cb) {
  return 'foo';
}.async()

var testSync = function() {
  Sync(function() {
    var places=[];

    console.time('getByIdAsync');
    // Load places
    for (var i=0; i < 2000; i++) {
      places.push(getByIdAsync.future(null, i));
    }
    console.timeEnd('getByIdAsync');

    console.time('getByIdCb');
    // Load places
    for (var i=0; i < 2000; i++) {
      places.push(getByIdCb.future(null, i));
    }
    console.timeEnd('getByIdCb');
  }, function(e) {
    if (e) {
      console.error(e.stack);
    }
  });
}

testSync(); 

This is the output: /server/node$ node profileSync.js getByIdAsync: 847ms getByIdCb: 575ms

ybogdanov commented 11 years ago

Hi,

Yep, sync.future could be slow. Please run this benchmark in your environment https://github.com/0ctave/node-sync/blob/master/benchmarks/index.js

Also, in a real life, you will never be calling 2000 empty functions in a row. Imagine those functions are doing some i/o etc with average response time 10ms. So, your test will take 10*2000 + 847 = 20847 - sync takes only 4% of this time. In most cases it's allowable trade-off in favor of comfortable development and readable code.

In other cases (some async intensive stuff with very fast responses) to achieve the best performance nodejs could give, you should not use sync.

weizhu commented 11 years ago

Hi 0ctave, the sample code I used is using empty function only because I am doing pure perf measurement of of sync.future.

For my real world usage, I need to load 2000 objects and filter them. So I am writing a loop to load the objects in future first, before using them. With future use, my code can make batch loading of data from caching layer. The IO costs only 50-100ms, but the rest of the loading code took 5 seconds!

I believe my usage is a reasonable scenario where sync.future should be used to in order to archive maximum IO performance. Also, I believe that there should be a way to optimize node-sync to dramatically improve its performance. For example, I tried to re-write the sample code in Fibers future (see https://gist.github.com/weizhu/7613409) and it took only 44 ms. That's 10-20x faster than sync.future.