You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/06/25 20:47:42 UTC
[kudu-CR] locks: add new read-write mutex
Hello Dan Burkert, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/3496
to review the following change.
Change subject: locks: add new read-write mutex
......................................................................
locks: add new read-write mutex
The new mutex is a thin wrapper around pthread read/write locks. It has no
features of which to speak: no debugging hooks, no optimizations, nothing.
Take these rwlock-perf results with a grain of salt; they only test
read/read contention, not read/write or write/write contention. The values
are millions of cycles.
num_threads laptop_old laptop_new ve0518_old ve0518_new
-------------------------------------------------------
1 261 83 189 82
2 1604 644 914 730
3 1504 917 2204 1567
4 2398 1792 3185 2162
5 3210 2179 3943 2308
6 4070 2696 4135 2515
7 4741 3253 4557 2732
8 5457 3853 5114 3145
Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
---
M src/kudu/experiments/rwlock-perf.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/rw_mutex.cc
A src/kudu/util/rw_mutex.h
4 files changed, 115 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/3496/1
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 4:
(1 comment)
> (1 comment)
>
> LGTM with some concern regarding error detection if something goes
> wrong.
>
> So, we are about to catch some those non-run-time issues like
> trying to acquire the lock held by the same thread, etc. in debug
> mode, which is OK.
>
> Do we expect to catch some run-time issues like lack of shared
> memory to accommodate a new lock, etc.? I.e. we are not
> propagating errors from pthread_rwlock_xxx() functions in release
> mode at all (if I'm not missing something). Does it make sense to
> throw some sort of exception for run-time errors (like
> std::runtime_error)?
It probably does (I'm not 100% sure, would need to give it more thought), but I'd rather keep that out of scope of this patch. The behavior of this new lock is consistent with that of our other pthread-based locks.
If we're going to address the issue of runtime checking of pthread return values, I think it makes sense to do it across the board in a separate patch.
http://gerrit.cloudera.org:8080/#/c/3496/4/src/kudu/util/mutex.cc
File src/kudu/util/mutex.cc:
Line 92: ; // NOLINT(whitespace/semicolon)
> As Dan already mentioned: is it possible to keep the semicolon under the if
Yes, but I actually messed up when I published this version of the diff. I meant to pull the DCHECK_EQ() out of the NDEBUG block. See the next revision.
If it's pulled out, it's no longer possible to put the semicolon on the end of the line, at least not without duplicating the DCHECK_EQ().
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/3496/2/src/kudu/util/rw_mutex.cc
File src/kudu/util/rw_mutex.cc:
Line 26: DCHECK_EQ(0, rv) << strerror(rv);
> does this cause "unused" warnings in release builds? perhaps need to use at
Yes it does. As do the unused 'rv' in Mutex and ConditionVariable.
I'll fix up all of them.
Line 39: DCHECK_EQ(0, rv) << strerror(rv);
> same here and below
Done
Line 43: int rv = pthread_rwlock_rdlock(&native_handle_);
> reading the manpage, it seems to me some of the errors could possibly occur
Yes, I think we generally expect these to be caught in testing. I was following the Mutex and ConditionVariable implementations.
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3496/2/src/kudu/util/rw_mutex.cc
File src/kudu/util/rw_mutex.cc:
Line 34: pthread_rwlock_init(&native_handle_, NULL);
Does it make sense to check for return value here as well?
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/1996/
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 1: Verified+1
Overriding Jenkins, there was some inscrutable error in test setup for org.kududb.client.TestScanPredicate that is definitely not the doing of this patch.
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3496/5/src/kudu/util/mutex.cc
File src/kudu/util/mutex.cc:
Line 87: DCHECK_EQ(0, rv) << ". " << strerror(rv)
> Ok fair enough. Final comment: could this be folded back into the original
If I move it into the #ifdef, the unused variable warning for 'rv' will resurface.
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: locks: add new read-write mutex
......................................................................
locks: add new read-write mutex
The new mutex is a thin wrapper around pthread read/write locks. It has no
features of which to speak: no debugging hooks, no optimizations, nothing.
Take these rwlock-perf results with a grain of salt; they only test
read/read contention, not read/write or write/write contention. The values
are millions of cycles.
num_threads laptop_old laptop_new ve0518_old ve0518_new
-------------------------------------------------------
1 261 83 189 82
2 1604 644 914 730
3 1504 917 2204 1567
4 2398 1792 3185 2162
5 3210 2179 3943 2308
6 4070 2696 4135 2515
7 4741 3253 4557 2732
8 5457 3853 5114 3145
Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Reviewed-on: http://gerrit.cloudera.org:8080/3496
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/experiments/rwlock-perf.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/mutex.cc
A src/kudu/util/rw_mutex.cc
A src/kudu/util/rw_mutex.h
5 files changed, 122 insertions(+), 5 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3496
to look at the new patch set (#4).
Change subject: locks: add new read-write mutex
......................................................................
locks: add new read-write mutex
The new mutex is a thin wrapper around pthread read/write locks. It has no
features of which to speak: no debugging hooks, no optimizations, nothing.
Take these rwlock-perf results with a grain of salt; they only test
read/read contention, not read/write or write/write contention. The values
are millions of cycles.
num_threads laptop_old laptop_new ve0518_old ve0518_new
-------------------------------------------------------
1 261 83 189 82
2 1604 644 914 730
3 1504 917 2204 1567
4 2398 1792 3185 2162
5 3210 2179 3943 2308
6 4070 2696 4135 2515
7 4741 3253 4557 2732
8 5457 3853 5114 3145
Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
---
M src/kudu/experiments/rwlock-perf.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/mutex.cc
A src/kudu/util/rw_mutex.cc
A src/kudu/util/rw_mutex.h
5 files changed, 122 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/3496/4
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] locks: add new read-write mutex
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3496/2/src/kudu/util/rw_mutex.cc
File src/kudu/util/rw_mutex.cc:
Line 43: int rv = pthread_rwlock_rdlock(&native_handle_);
reading the manpage, it seems to me some of the errors could possibly occur in the wild:
[EDEADLK]
The current thread already owns the read-write lock for writing.
[EAGAIN]
The read lock could not be acquired because the maximum number of read locks for rwlock has been exceeded.
Although maybe we are betting on these cases being weeded out in testing?
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 2:
(1 comment)
LGTM with some concern regarding error detection if something goes wrong.
So, we are about to catch some those non-run-time issues like trying to acquire the lock held by the same thread, etc. in debug mode, which is OK.
Do we expect to catch some run-time issues like lack of shared memory to accommodate a new lock, etc.? I.e. we are not propagating errors from pthread_rwlock_xxx() functions in release mode at all (if I'm not missing something). Does it make sense to throw some sort of exception for run-time errors (like std::runtime_error)?
http://gerrit.cloudera.org:8080/#/c/3496/4/src/kudu/util/mutex.cc
File src/kudu/util/mutex.cc:
Line 92: }
As Dan already mentioned: is it possible to keep the semicolon under the ifndef and in the same line as last statement?
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/1990/
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] locks: add new read-write mutex
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/2063/
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3496
to look at the new patch set (#5).
Change subject: locks: add new read-write mutex
......................................................................
locks: add new read-write mutex
The new mutex is a thin wrapper around pthread read/write locks. It has no
features of which to speak: no debugging hooks, no optimizations, nothing.
Take these rwlock-perf results with a grain of salt; they only test
read/read contention, not read/write or write/write contention. The values
are millions of cycles.
num_threads laptop_old laptop_new ve0518_old ve0518_new
-------------------------------------------------------
1 261 83 189 82
2 1604 644 914 730
3 1504 917 2204 1567
4 2398 1792 3185 2162
5 3210 2179 3943 2308
6 4070 2696 4135 2515
7 4741 3253 4557 2732
8 5457 3853 5114 3145
Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
---
M src/kudu/experiments/rwlock-perf.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/mutex.cc
A src/kudu/util/rw_mutex.cc
A src/kudu/util/rw_mutex.h
5 files changed, 122 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/3496/5
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] locks: add new read-write mutex
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3496/5/src/kudu/util/mutex.cc
File src/kudu/util/mutex.cc:
Line 87: MicrosecondsInt64 end_time = GetMonoTimeMicros();
I still don't really get what this is about. Is the point just to avoid an unused variable warning on 'rv'? If so, why not just NOLINT that directly instead of jumping through these hoops.
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3496/5/src/kudu/util/mutex.cc
File src/kudu/util/mutex.cc:
Line 87: DCHECK_EQ(0, rv) << ". " << strerror(rv)
> The problem is that it's not lint that's complaining about the unused varia
Ok fair enough. Final comment: could this be folded back into the original ifdef?
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3496/5/src/kudu/util/mutex.cc
File src/kudu/util/mutex.cc:
Line 87: DCHECK_EQ(0, rv) << ". " << strerror(rv)
> I still don't really get what this is about. Is the point just to avoid an
The problem is that it's not lint that's complaining about the unused variable; it's the compiler. Doing it this way seemed about as intrusive as using __attribute__ ((unused)) to me, so I went with the approach with less cognitive load (i.e. no new concepts).
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3496
to look at the new patch set (#3).
Change subject: locks: add new read-write mutex
......................................................................
locks: add new read-write mutex
The new mutex is a thin wrapper around pthread read/write locks. It has no
features of which to speak: no debugging hooks, no optimizations, nothing.
Take these rwlock-perf results with a grain of salt; they only test
read/read contention, not read/write or write/write contention. The values
are millions of cycles.
num_threads laptop_old laptop_new ve0518_old ve0518_new
-------------------------------------------------------
1 261 83 189 82
2 1604 644 914 730
3 1504 917 2204 1567
4 2398 1792 3185 2162
5 3210 2179 3943 2308
6 4070 2696 4135 2515
7 4741 3253 4557 2732
8 5457 3853 5114 3145
Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
---
M src/kudu/experiments/rwlock-perf.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/mutex.cc
A src/kudu/util/rw_mutex.cc
A src/kudu/util/rw_mutex.h
5 files changed, 122 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/3496/3
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] locks: add new read-write mutex
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3496/2/src/kudu/util/rw_mutex.cc
File src/kudu/util/rw_mutex.cc:
Line 26: DCHECK_EQ(0, rv) << strerror(rv);
does this cause "unused" warnings in release builds? perhaps need to use attribute unused or somesuch
Line 39: DCHECK_EQ(0, rv) << strerror(rv);
same here and below
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3496/2/src/kudu/util/rw_mutex.cc
File src/kudu/util/rw_mutex.cc:
Line 26: DCHECK_EQ(0, rv) << strerror(rv);
> Yes it does. As do the unused 'rv' in Mutex and ConditionVariable.
Quick note: these don't cause unused warnings, at least not with gcc 5.3. There was one warning in mutex.cc; I've fixed it.
Line 34: pthread_rwlock_init(&native_handle_, NULL);
> Does it make sense to check for return value here as well?
The manpage says it should never return non-zero, but I've added the check anyway; it doesn't hurt and is more defensive.
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] locks: add new read-write mutex
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] locks: add new read-write mutex
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/2057/
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] locks: add new read-write mutex
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 5:
Build Started http://104.196.14.100/job/kudu-gerrit/2068/
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] locks: add new read-write mutex
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: locks: add new read-write mutex
......................................................................
Patch Set 5: -Verified
Build Started http://104.196.14.100/job/kudu-gerrit/2075/
--
To view, visit http://gerrit.cloudera.org:8080/3496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No