improve work queue

[toffaletti]

Review request. Relates to #4877. Proposing this as a more conservative alternative to Chase-Lev until I figure out how to get array growth and memory reclamation working without a garbage collector.

[emberian]

the license block needs to stay

[emberian]

cc @Aatch @brson @bblum

[Aatch]

Can you try to add a test that exercises the concurrent aspect?

[alexcrichton]

It troubles me that there are 0 comments in this code. Concurrent data structures are hard enough before you add lock-free components to them. Can you please add some comments explaining what's going on and where?

Additionally, the commit message "improve work queue" isn't very descriptive, can you expand that with a title and a description as to what was improved as well?

[alexcrichton]

As with the previous queues, I'm wary of having a "safe way to use these unsafely." I would much rather have a safe interface exposing the invariants of this queue. I quickly got tripped up over myself thinking that there were multiple pushers/poppers.

This is an excellent candidate for a WorkQueue and a WorkQueueHandle where only one of the two is clone-able. It seems that there is a clear way to integrate this into the scheduler, and I think that it would be much cleaner than just hoping that the consumer of this queue doesn't have two pushers/poppers.

[alexcrichton]

Oh, I forgot to mention, but do you have any numbers on this queue about how the performance looks? I'm wary of using a try lock to steal work because it seems like if work queues are under high contention no one will end up stealing anything when there's lots of work to steal.

[toffaletti]

I agree using the type system to enforce correct usage is a good idea. I'm wary of changing too many things at once though so I'd like to do the API changes in another patch.

I ran some benchmarks on a C++ version of this and the performance seemed good, and comparable to the Chase-Lev implementation I was working on which is why I decided to try it out.

The test_steal test I added can be used to verify the balance of work between threads. I have a stand-alone version at https://github.com/toffaletti/rust-code/blob/master/work_queue.rs which you can run with RUST_LOG=work_queue ./work_queue. The output should look something like this:

count: 133
count: 68
count: 106
count: 100
count: 127
count: 76
count: 92
count: 117
count: 181
test tests::test_steal ... ok

The test simulates work by doing a task::deschedule. I'd be interested in a better real-world test. In the past I tried using rust-http server and apache bench with different levels of concurrency, but it only utilizes a single core because of libuv+rust integration causing all IO to occur in a single thread.

As an aside, I've found it rather cumbersome to develop things inside libstd. Which is why I have stand-alone versions so I can just rustc --test work_queue.rs instead of having the overhead of make check-stage1 for every little change. There must be a better way?

Finally, I'll work on commenting the code and perhaps removing the mask magic, which is just a silly optimization a lot of these concurrent data structures backed by arrays use. If the size of the array is made to be a power of two, they can use indexCounter & mask instead of indexCounter % arraySize to get the real array index when indexCounter is an only atomically incrementing number because I guess bitwise AND is faster than modulus in silicon. My instinct doubts it makes a difference outside of synthetic benchmarks.

[emberian]

Can you add benchmarks to the tests, or in src/test/bench?

On Mon, Nov 18, 2013 at 5:01 PM, Jason Toffaletti
notifications@github.heygears.comwrote:

I agree using the type system to enforce correct usage is a good idea. I'm
wary of changing too many things at once though so I'd like to do the API
changes in another patch.

I ran some benchmarks on a C++ version of this and the performance seemed
good, and comparable to the Chase-Lev implementation I was working on which
is why I decided to try it out.

The test_steal test I added can be used to verify the balance of work
between threads. I have a stand-alone version at
https://github.com/toffaletti/rust-code/blob/master/work_queue.rs which
you can run with RUST_LOG=work_queue ./work_queue. The output should look
something like this:

count: 133
count: 68
count: 106
count: 100
count: 127
count: 76
count: 92
count: 117
count: 181
test tests::test_steal ... ok

The test simulates work by doing a task::deschedule. I'd be interested in
a better real-world test. In the past I tried using rust-http server and
apache bench with different levels of concurrency, but it only utilizes a
single core because of libuv+rust integration causing all IO to occur in a
single thread.

As an aside, I've found it rather cumbersome to develop things inside
libstd. Which is why I have stand-alone versions so I can just rustc
--test work_queue.rs instead of having the overhead of make check-stage1for every little change. There must be a better way?

Finally, I'll work on commenting the code and perhaps removing the mask
magic, which is just a silly optimization a lot of these concurrent data
structures backed by arrays use. If the size of the array is made to be a
power of two, they can use indexCounter & mask instead of indexCounter %
arraySize to get the real array index when indexCounter is an only
atomically incrementing number because I guess bitwise AND is faster than
modulus in silicon. My instinct doubts it makes a difference outside of
synthetic benchmarks.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/10529#issuecomment-28742539
.

[alexcrichton]

Closing now that #10678 has landed