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 2018/01/13 00:02:32 UTC

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads 

Hello Dan Burkert, Todd Lipcon,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/9021

to review the following change.


Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................

KUDU-1913: LIFO wake up ordering for threadpool worker threads

This patch changes the wake-up ordering for idle pool worker threads
to be LIFO. Previously all worker threads idled on a single
ConditionVariable which, by virtue of using pthread_cond_t under the hood,
was FIFO ordered.

In the abstract, FIFO ordering ensures a fair distribution of work amongst a
set of threads, but that's totally undesirable in a thread pool where the
goal is to get work done as quickly and with as few threads as possible. For
example, a fast stream of cheap tasks (e.g. RPCs) should be serviceable via
a single thread or something close to that.

The new unit test was looped 1000 times in TSAN mode with 8 stress threads
to shake out any flakes. Additionally, I ran through a variation of Todd's
test from a past code review [1]. I spun up three tservers, two of which had
failure detection disabled so that the third would get all of the leader
replicas. I created 240 tablets and looked at the size of the Raft thread
pool on that tserver. The steady state thread count was mildly lower at
first (4 vs. 8) but was much lower (4 vs. 30) after a SIGSTOP+SIGCONT cycle.

1. https://gerrit.cloudera.org/c/7331

Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
---
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
5 files changed, 114 insertions(+), 15 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/9021/1
-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................

KUDU-1913: LIFO wake up ordering for threadpool worker threads

This patch changes the wake-up ordering for idle pool worker threads
to be LIFO. Previously all worker threads idled on a single
ConditionVariable which, by virtue of using pthread_cond_t under the hood,
was FIFO ordered.

In the abstract, FIFO ordering ensures a fair distribution of work amongst a
set of threads, but that's totally undesirable in a thread pool where the
goal is to get work done as quickly and with as few threads as possible. For
example, a fast stream of cheap tasks (e.g. RPCs) should be serviceable via
a single thread or something close to that.

The new unit test was looped 1000 times in TSAN mode with 8 stress threads
to shake out any flakes. Additionally, I ran through a variation of Todd's
test from a past code review [1]. I spun up three tservers, two of which had
failure detection disabled so that the third would get all of the leader
replicas. I created 240 tablets and looked at the size of the Raft thread
pool on that tserver. The steady state thread count was mildly lower at
first (4 vs. 8) but was much lower (4 vs. 30) after a SIGSTOP+SIGCONT cycle.

1. https://gerrit.cloudera.org/c/7331

Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Reviewed-on: http://gerrit.cloudera.org:8080/9021
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
5 files changed, 114 insertions(+), 15 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Todd Lipcon: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................


Patch Set 1: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/test_util.h@115
PS1, Line 115:   // Do not use any back-off and loop every millisecond.
nit: it doesn't check every millisecond, but rather sleeps one millisecond between checks (which might result in checking less frequently in the case that the checks themselves take some time)


http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool-test.cc@934
PS1, Line 934: [](){}
> o_O
at least he didn't use the equivalent syntax  <:]{%>


http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool.cc@621
PS1, Line 621:       auto cleanup = MakeScopedCleanup([&]() {
can you use the SCOPED_CLEANUP macro instead if you don't need the named variable for cancellation purposes?



-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 05:48:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................


Patch Set 2: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 23:26:00 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/9021

to look at the new patch set (#2).

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................

KUDU-1913: LIFO wake up ordering for threadpool worker threads

This patch changes the wake-up ordering for idle pool worker threads
to be LIFO. Previously all worker threads idled on a single
ConditionVariable which, by virtue of using pthread_cond_t under the hood,
was FIFO ordered.

In the abstract, FIFO ordering ensures a fair distribution of work amongst a
set of threads, but that's totally undesirable in a thread pool where the
goal is to get work done as quickly and with as few threads as possible. For
example, a fast stream of cheap tasks (e.g. RPCs) should be serviceable via
a single thread or something close to that.

The new unit test was looped 1000 times in TSAN mode with 8 stress threads
to shake out any flakes. Additionally, I ran through a variation of Todd's
test from a past code review [1]. I spun up three tservers, two of which had
failure detection disabled so that the third would get all of the leader
replicas. I created 240 tablets and looked at the size of the Raft thread
pool on that tserver. The steady state thread count was mildly lower at
first (4 vs. 8) but was much lower (4 vs. 30) after a SIGSTOP+SIGCONT cycle.

1. https://gerrit.cloudera.org/c/7331

Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
---
M src/kudu/util/test_util.cc
M src/kudu/util/test_util.h
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
5 files changed, 114 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/9021/2
-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool-test.cc@934
PS1, Line 934: [](){}
o_O



-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 01:28:47 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/test_util.h
File src/kudu/util/test_util.h:

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/test_util.h@115
PS1, Line 115:   // Sleep for a millisecond while looping.
> nit: it doesn't check every millisecond, but rather sleeps one millisecond 
Thanks, will clarify.


http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:

http://gerrit.cloudera.org:8080/#/c/9021/1/src/kudu/util/threadpool.cc@621
PS1, Line 621:       SCOPED_CLEANUP({
> can you use the SCOPED_CLEANUP macro instead if you don't need the named va
I had been cancelling at one point, then didn't need to anymore but forgot to revert back to SCOPED_CLEANUP. Done.



-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 22:30:04 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1913: LIFO wake up ordering for threadpool worker threads

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9021 )

Change subject: KUDU-1913: LIFO wake up ordering for threadpool worker threads
......................................................................


Patch Set 1: Verified+1

One ASAN test timed out in table creation, which I think is unrelated to this commit.


-- 
To view, visit http://gerrit.cloudera.org:8080/9021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8036bf0d15f9ffcb3fa76579e3bfe0a340d38320
Gerrit-Change-Number: 9021
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 13 Jan 2018 00:58:49 +0000
Gerrit-HasComments: No