[kudu-CR] locks: add new read-write mutex
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
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 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 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 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
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
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 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 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 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 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
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 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 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 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
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 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
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
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 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/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 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 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 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 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 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
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 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 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 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
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 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 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
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 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
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 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
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