impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Date Mon, 12 Sep 2016 16:46:39 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Chen Huang <paulhuang34@utexas.edu>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message