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

Added steed to benchmarks #887

Closed
wants to merge 3 commits into from
Closed

Conversation

mcollina
Copy link

I've added steed to the benchmarks.

It takes a different approach compared to neo-async, and possibly the performance might be different. It recycles functions, so they can be optimized (vs allocating new functions). In my tests, performance is similar or better (and it does not pollute --trace_opt): here it comes slightly lower, and I still have to investigate what is happening.

Anyway, it is worth benchmarking.

BTW, I have found that the benchmarks are more favorable to promise-based apps, because they allocates a lot of functions, which in most cases are not needed in a callback-based implementation.

@mcollina
Copy link
Author

I've added a couple more benchmarks that are more optimized for callbacks. On my box:

file                                      time(ms)  memory(MB)
callbacks-baseline.js                          337       49.33
callbacks-mcollina-fastparallel.js             391       71.87
promises-bluebird-generator.js                 407       78.14
promises-bluebird.js                           461       74.73
callbacks-mcollina-steed-map.js                488       72.15
callbacks-mcollina-steed-parallel.js           545      116.71
callbacks-suguru03-neo-async-parallel.js       553      100.25

I originally thought to name my lib ludicrous :D.

(The picture is copyright by Disney I presume).

@benjamingr
Copy link
Collaborator

@spion can you please review as this is benchmark code?

var blob = blobManager.create(account);
var tx = db.begin();
var blobId, file, version, fileId;
async.waterfall([
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is still trying to use async instead of steed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol ^_^

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed :D.

@spion
Copy link
Collaborator

spion commented Jan 6, 2016

Looks okay to me, but I haven't had a chance to test whether everything executes correctly. The trouble with the benchmark code is that there is no verification that all implementations ultimately do the same thing...

@benjamingr
Copy link
Collaborator

Thanks, I think we can merge - @petkaantonov ?

@benjamingr
Copy link
Collaborator

@petkaantonov

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.

4 participants