[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

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   26183 18982
2   1604   644914730
3   1504   9172204   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 
---
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] locks: add new read-write mutex

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] locks: add new read-write mutex

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

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   26183 18982
2   1604   644914730
3   1504   9172204   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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] locks: add new read-write mutex

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

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   26183 18982
2   1604   644914730
3   1504   9172204   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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

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(_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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] locks: add new read-write mutex

[Dan Burkert (Code Review)]

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(_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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] locks: add new read-write mutex

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

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   26183 18982
2   1604   644914730
3   1504   9172204   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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon