[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.

["Tim Armstrong (Code Review)" <ge...@cloudera.org>]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................


Patch Set 2:

(5 comments)

Thanks for switching back to unique_lock, it makes it a bit easier to reason about for me. Change looks good, just a few comments.

http://gerrit.cloudera.org:8080/#/c/4350/2/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:

Line 76:           // Sleep with read lock held to block off other readers which cannot
Won't this change the meaning of total_get_wait_time_ so that we're only counting the single thread that got the lock?

This is ok since I don't think the counter is contractual, and the new definition may be more useful, but should update the comment.


Line 111:     write_lock.unlock();
Not your change but it'd be more consistent to use braced scoping to release the lock instead of this explicit unlock.


Line 155:   uint32_t GetSize() const {
It looks like this is sometimes called with lock held or not. Seems like we should document this (i.e. caller should hold lock for non-racy read).


Line 161:   uint64_t total_get_wait_time() const {
It doesn't look like total_get_wait_time and total_put_wait_time have any callers - could just get rid ofthe functions?


Line 189:   boost::mutex write_lock_;
We might be able to further reduce contention if we align the locks and other data structures so that they're guaranteed to not share 64-bit cache lines. Maybe group read and write members together so that the can share cache lines, then align the first member of both groups to 64 bytes?

They're only 40 bytes on my system, so with the current layout read_lock and write_lock may be on the same cache line:

    #include <pthread.h>
    #include <stdio.h>
    #include <stddef.h>
    #include <boost/thread/mutex.hpp>


    struct test {
      boost::mutex mutex1;
      boost::mutex mutex2;
    };

    int main() {
      printf("sizeof(pthread_mutex_t)=%ld\n", sizeof(pthread_mutex_t));
      printf("sizeof(boost::mutex)=%ld\n", sizeof(boost::mutex));
      printf("sizeof(pthread_cond_t)=%ld\n", sizeof(pthread_cond_t));
      printf("offsetof(...)=%ld\n", offsetof(struct test, mutex2));
      return 0;
    }


    [~]$ g++ mutex.cc -lboost_system -o mutex&& ./mutex                            
    sizeof(pthread_mutex_t)=40
    sizeof(boost::mutex)=40
    sizeof(pthread_cond_t)=48
    offsetof(...)=40
    [~]$


-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes