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

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 17 Jan 2018 23:26:00 +
Gerrit-HasComments: No

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

[Todd Lipcon (Code Review)]

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

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

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 17 Jan 2018 22:30:04 +
Gerrit-HasComments: Yes

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

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 17 Jan 2018 05:48:08 +
Gerrit-HasComments: Yes

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 17 Jan 2018 01:28:47 +
Gerrit-HasComments: Yes

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

[Adar Dembo (Code Review)]

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

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

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 13 Jan 2018 00:58:49 +
Gerrit-HasComments: No

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

[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/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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon