[concurrent-blocking-queue-fix] ConcurrentBlockingQueue::Pop's return…

[hotpxl]

ConcurrentBlockingQueue::Pop's return value's semantic was unconventional in Minerva. Correct it here

Since it is used in the engine, @jermainewang you might want to know the change

[tqchen]

There are a few more things we can do:

• Try to use c++11's atomic to implement the spin_lock
  • This may solve some ASM issues across archs
• Put things in namespace mxnet::common, heavily document all the class variables and functions in the header(refer to other headers in include)
  • So others can use it easily(we might move them to dmlc-core eventually)

[tqchen]

Let us also loop @mli in this on possibility of moving additional thread concurrency util into dmlc-core. we could possibly put them in include/dmlc/thread

[jermainewang]

For the spin_lock implementation, I think current implementation is quite
efficient. The "xchg" instruction is atomic instruction. I don't know
c++11's implementation. But why we need to use 11's atomic to implement
that?

On Wed, Jul 1, 2015 at 12:11 PM, Tianqi Chen notifications@github.com
wrote:

Let us also loop @mli https://github.com/mli in this on possibility of
moving additional thread concurrency util into dmlc-core. we could possibly
put them in include/dmlc/thread

—
Reply to this email directly or view it on GitHub
#2 (comment).

Minjie Wang
New York University | Computer Science
715 Broadway, New York, NY, 10009

[mli]

could you please evaluate the performance comparing to the following implementation?

#include <atomic>
class SpinLock {
public:
    void lock()  {
        while(lck.test_and_set(std::memory_order_acquire))
        {}
    }

    void unlock()   {
        lck.clear(std::memory_order_release);
    }

private:
    std::atomic_flag lck = ATOMIC_FLAG_INIT;
};

[tqchen]

@jermainewang The major reason was portability. ASM can be tricky when you port it to different platforms, for example, a architecture that do not support x86

[hotpxl]

@mli I tried that some time ago. It was no faster that std::atomic_bool. But nevertheless we could use that implementation. I couldn't be worse.

[tqchen]

@hotpxl if the efficiency is the same. I would recommend using atomic, due to the reason I mentioned

[hotpxl]

@tqchen I was comparing std::atomic_bool to std::atomic_flag. I never tested xchg.

[tqchen]

close since this is moved to dmlc-core