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

@hargasinski hargasinski commented Jun 7, 2017

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

@hargasinski hargasinski commented Jun 7, 2017

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

@aearly aearly Jun 8, 2017

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

@hargasinski hargasinski Jun 11, 2017

Choose a reason for hiding this comment

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

Added, thanks!

megawac
megawac approved these changes Jun 9, 2017
@hargasinski hargasinski merged commit 0364ff3 into master Jun 11, 2017
3 checks passed
@hargasinski hargasinski deleted the queue-workerslist-fix branch Jun 11, 2017
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.

None yet

3 participants