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

ensure q.workersList() contains items being processed [fixes #1428] #1429

Merged
merged 3 commits into from
Jun 11, 2017

Conversation

hargasinski
Copy link
Collaborator

Aligns q.workersList() and cargo.workersList() with the docs.

workersList() is not really relevant in the context of async.cargo, as there will only be 1 worker running at a given time, but I would prefer to keep the behavior consistent between it and queue.

@hargasinski hargasinski self-assigned this Jun 7, 2017
@hargasinski
Copy link
Collaborator Author

Also, just in case, I ran the benchmarks for queue. There doesn't appear to be a difference.

$ node perf/benchmark.js --grep queue
Latest tag is  v2.4.1
Comparing v2.4.1 with current on Node v7.4.0
--------------------------------------
queue(1000) v2.4.1 x 581 ops/sec ±1.52% (31 runs sampled), 1.72ms per run
queue(1000) current x 606 ops/sec ±0.69% (32 runs sampled), 1.65ms per run
current is faster
--------------------------------------
queue(30000) v2.4.1 x 16.96 ops/sec ±2.82% (30 runs sampled), 59.0ms per run
queue(30000) current x 15.94 ops/sec ±2.28% (29 runs sampled), 62.7ms per run
v2.4.1 is faster
--------------------------------------
queue(100000) v2.4.1 x 4.11 ops/sec ±10.95% (9 runs sampled), 243ms per run
queue(100000) current x 4.22 ops/sec ±6.17% (9 runs sampled), 237ms per run
Tie
--------------------------------------
queue(200000) v2.4.1 x 2.10 ops/sec ±13.67% (5 runs sampled), 477ms per run
queue(200000) current x 2.07 ops/sec ±11.86% (5 runs sampled), 482ms per run
Tie
--------------------------------------
Both versions are about equal (781ms total vs. 784ms total)
Both versions won the same number of benchmarks (1 vs. 1)

}

var q = async.queue(function(task, cb) {
expect(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also assert that workersList().length === running() here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added, thanks!

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

Successfully merging this pull request may close these issues.

3 participants