You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/06/24 23:59:05 UTC
[kudu-CR] replace boost::detail::yield with std::this thread::yield
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/3492
to review the following change.
Change subject: replace boost::detail::yield with std::this_thread::yield
......................................................................
replace boost::detail::yield with std::this_thread::yield
source of boost::detail::yield:
https://github.com/boostorg/smart_ptr/blob/boost-1.59.0/include/boost/smart_ptr/detail/yield_k.hpp#L116-L144
source of std::this_thread::yield:
https://github.com/gcc-mirror/gcc/blob/gcc_5_3_0_release/libstdc%2B%2B-v3/include/std/thread#L265-L272
(boils down to a call to sched_yield(2)).
Change-Id: I1c94f8377982028e608c0b0d0b56a48f8df4dfe2
---
M src/kudu/tablet/concurrent_btree.h
M src/kudu/util/rw_semaphore.h
M src/kudu/util/thread.cc
M src/kudu/util/threadpool-test.cc
4 files changed, 14 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/3492/1
--
To view, visit http://gerrit.cloudera.org:8080/3492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1c94f8377982028e608c0b0d0b56a48f8df4dfe2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] replace boost::detail::yield with std::this thread::yield
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: replace boost::detail::yield with std::this_thread::yield
......................................................................
Patch Set 1:
This isn't entirely equivalent since I removed the spin counters. We could put those back in, but it's more effort. Not sure exactly how boost uses them.
--
To view, visit http://gerrit.cloudera.org:8080/3492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c94f8377982028e608c0b0d0b56a48f8df4dfe2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] replace boost::detail::yield with std::this thread::yield
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has abandoned this change.
Change subject: replace boost::detail::yield with std::this_thread::yield
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/3492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I1c94f8377982028e608c0b0d0b56a48f8df4dfe2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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>
[kudu-CR] replace boost::detail::yield with std::this thread::yield
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: replace boost::detail::yield with std::this_thread::yield
......................................................................
Patch Set 1:
> I dont think this is a good idea - we use boost yield in some cases
> where we might spin a pretty long time and spinning on
> sched_yield() would suck up lots of kernel CPU (it reschedules the
> same thread if there's nothing else to run)
It doesn't seem like it'd be too much work to implement our own adaptive yield. The boost implementation is small.
--
To view, visit http://gerrit.cloudera.org:8080/3492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c94f8377982028e608c0b0d0b56a48f8df4dfe2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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] replace boost::detail::yield with std::this thread::yield
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: replace boost::detail::yield with std::this_thread::yield
......................................................................
Patch Set 1:
Dan, do you still want to make a case for this?
--
To view, visit http://gerrit.cloudera.org:8080/3492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c94f8377982028e608c0b0d0b56a48f8df4dfe2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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] replace boost::detail::yield with std::this thread::yield
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: replace boost::detail::yield with std::this_thread::yield
......................................................................
Patch Set 1: Code-Review-1
I dont think this is a good idea - we use boost yield in some cases where we might spin a pretty long time and spinning on sched_yield() would suck up lots of kernel CPU (it reschedules the same thread if there's nothing else to run)
--
To view, visit http://gerrit.cloudera.org:8080/3492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c94f8377982028e608c0b0d0b56a48f8df4dfe2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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] replace boost::detail::yield with std::this thread::yield
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: replace boost::detail::yield with std::this_thread::yield
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/1986/
--
To view, visit http://gerrit.cloudera.org:8080/3492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c94f8377982028e608c0b0d0b56a48f8df4dfe2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No