You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/10/11 04:08:17 UTC
[kudu-CR] thread: improve performance of starting threads
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8257
to review the following change.
Change subject: thread: improve performance of starting threads
......................................................................
thread: improve performance of starting threads
This improves the performance of starting a new thread using
kudu::Thread::StartThread(). Previously, the process of starting the
thread was a little bit more complicated than necessary, involving a
sequence of the child waiting for the parent to do something, then the
parent waiting for the child to do something. Now, neither one has to
wait for the other to proceed.
The one wrinkle here is that for the parent to know the child's tid, it
does have to wait for it to publish it. However, usage of the 'tid()'
function is relatively rare, so we can defer that waiting until someone
accesses it, and then only wait in the odd case that it hasn't yet
published.
This patch includes a very simple benchmark which starts 1000 threads
and then checks the pid for each.
Timings based on the 'Time spent' log output:
---------------------------------------------
Before:
wall user sys
Min. :0.06500 Min. :0.0080 Min. :0.0160
1st Qu.:0.06975 1st Qu.:0.0160 1st Qu.:0.0200
Median :0.07100 Median :0.0200 Median :0.0240
Mean :0.07080 Mean :0.0198 Mean :0.0234
3rd Qu.:0.07200 3rd Qu.:0.0240 3rd Qu.:0.0280
Max. :0.08200 Max. :0.0280 Max. :0.0320
After:
wall user sys
Min. :0.01800 Min. :0.0040 Min. :0.000
1st Qu.:0.02575 1st Qu.:0.0080 1st Qu.:0.012
Median :0.02700 Median :0.0120 Median :0.016
Mean :0.02680 Mean :0.0126 Mean :0.016
3rd Qu.:0.02925 3rd Qu.:0.0160 3rd Qu.:0.020
Max. :0.03200 Max. :0.0280 Max. :0.028
perf-stat results
------------------
Before:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
1095.617237 task-clock (msec) # 0.863 CPUs utilized ( +- 0.54% )
29,552 context-switches # 0.027 M/sec ( +- 0.75% )
354 cpu-migrations # 0.323 K/sec ( +- 4.55% )
20,848 page-faults # 0.019 M/sec ( +- 0.11% )
2,424,700,945 cycles # 2.213 GHz ( +- 0.35% )
1,369,266,675 instructions # 0.56 insn per cycle ( +- 0.12% )
286,851,480 branches # 261.817 M/sec ( +- 0.11% )
5,926,839 branch-misses # 2.07% of all branches ( +- 0.39% )
1.269172342 seconds time elapsed ( +- 0.33% )
After:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
790.618424 task-clock (msec) # 1.677 CPUs utilized ( +- 0.77% )
17,178 context-switches # 0.022 M/sec ( +- 0.91% )
3,869 cpu-migrations # 0.005 M/sec ( +- 0.76% )
20,921 page-faults # 0.026 M/sec ( +- 0.05% )
2,149,909,940 cycles # 2.719 GHz ( +- 0.41% )
1,149,982,930 instructions # 0.53 insn per cycle ( +- 0.09% )
239,776,764 branches # 303.277 M/sec ( +- 0.08% )
4,890,927 branch-misses # 2.04% of all branches ( +- 0.62% )
0.471358038 seconds time elapsed ( +- 0.76% )
Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
---
M src/kudu/util/thread-test.cc
M src/kudu/util/thread.cc
M src/kudu/util/thread.h
3 files changed, 86 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/8257/1
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] thread: improve performance of starting threads
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8257 )
Change subject: thread: improve performance of starting threads
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@591
PS2, Line 591:
> well, it's only in VLOG, so basically if --v=2 is enabled we'll lose the pe
Right, I remembered about the lazy evaluation, just thought we wouldn't want to wait here in the --v=2 case because we'd lose potentially interesting thread interleavings.
But it doesn't matter.
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 13 Oct 2017 01:40:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] thread: improve performance of starting threads
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8257 )
Change subject: thread: improve performance of starting threads
......................................................................
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/client/client-test.cc@4805
PS2, Line 4805: SleepFor(MonoDelta::FromMilliseconds(100));
> Seems like the ideal value here would be different for TSAN/ASAN than for r
I guess the thinking is that we don't really know how many client threads are sufficient to drive the load required to overrun the server. I didn't want to redesign the test here, so just tweaked it to avoid the issue where threads started too fast.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@a589
PS2, Line 589:
> Curious why you moved this out of here and into StartThread?
I hit an odd TSAN race with detached threads and thread destructors -- something where I'd started a thread and then exited main(), which ran the ThreadManager destrcutor, and TSAN reported that as not-synchronized-with the SuperviseThread function.
I think we used to get some 'happens-before" by the synchronization on the tid_ variable but lost it, so that race surfaced itself.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@456
PS2, Line 456: Status ThreadJoiner::Join() {
: if (Thread::current_thread() &&
: Thread::current_thread()->tid() == thread_->tid()) {
:
> Given tid()'s new behavior, should we demote this to a DCHECK?
I think it's fine as is, because either:
(a) the calling thread is the current thread, in which case the tid_ would already have been assigned (because we don't call the user functor until after we've assigned it)
or:
(b) the calling thread is not the current thread, in which case we're going to proceed to join() anyway, so regardless we would block until the thread has truly started (since we're going to block until it finishes).
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@517
PS2, Line 517:
> I presume we don't have ToString() in any hot path?
yea I don't think so - only in logging, etc, where the few milliseconds we might need to wait for a thread to start isn't a big deal. Assumption is that worst case we're just *moving* a bit of wait time from start until the first access to the Thread object, on the optimistic assumption that we rarely actually access the tid() immediately after starting anyway.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@553
PS2, Line 553: *holder = t;
> Surely there's no use case for this? 'holder' is supposed to be a pure OUT
haha, I wrote it that way first, but then said to myself "I bet Adar is going to nit-pick this that you shouldn't modify out parameters if you return an unsuccessful status!"
But, I agree with you. I'll just change it to simplify the code.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@575
PS2, Line 575: {
> Could you use a MakeScopedCleanup for this, and cancel it on success?
Done
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@591
PS2, Line 591:
> This doesn't seem like something we'd want here, given tid()'s new behavior
well, it's only in VLOG, so basically if --v=2 is enabled we'll lose the perf optimization. But the cost of writing a log message is already pretty high so we aren't perf-sensitive in that case.
Keep in mind that VLOG lazy-evaluates its arguments.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@617
PS2, Line 617: string name = strings::Substitute("$0-$1", t->name(), system_tid);
: thread_manager->SetThreadName(name, t->tid_);
> Could use tid_ here instead.
Done
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@646
PS2, Line 646:
> Could use tid_ here instead too.
Done
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@648
PS2, Line 648: t->Release();
> Maybe add a brief comment explaining that this could cause t to destruct, s
Done
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 13 Oct 2017 01:10:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] thread: improve performance of starting threads
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8257 )
Change subject: thread: improve performance of starting threads
......................................................................
Patch Set 4:
(1 comment)
oops, missed one!
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread-test.cc
File src/kudu/util/thread-test.cc:
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread-test.cc@123
PS2, Line 123: std::vector<scoped_refptr<Thread>> threads(1000);
> ~Thread doesn't Join(), so shouldn't we explicitly join at the end of the t
Done
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 13 Oct 2017 01:11:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] thread: improve performance of starting threads
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8257
to look at the new patch set (#4).
Change subject: thread: improve performance of starting threads
......................................................................
thread: improve performance of starting threads
This improves the performance of starting a new thread using
kudu::Thread::StartThread(). Previously, the process of starting the
thread was a little bit more complicated than necessary, involving a
sequence of the child waiting for the parent to do something, then the
parent waiting for the child to do something. Now, neither one has to
wait for the other to proceed.
The one wrinkle here is that for the parent to know the child's tid, it
does have to wait for it to publish it. However, usage of the 'tid()'
function is relatively rare, so we can defer that waiting until someone
accesses it, and then only wait in the odd case that it hasn't yet
published.
This patch includes a very simple benchmark which starts 1000 threads
and then checks the pid for each.
Timings based on the 'Time spent' log output:
---------------------------------------------
Before:
wall user sys
Min. :0.06500 Min. :0.0080 Min. :0.0160
1st Qu.:0.06975 1st Qu.:0.0160 1st Qu.:0.0200
Median :0.07100 Median :0.0200 Median :0.0240
Mean :0.07080 Mean :0.0198 Mean :0.0234
3rd Qu.:0.07200 3rd Qu.:0.0240 3rd Qu.:0.0280
Max. :0.08200 Max. :0.0280 Max. :0.0320
After:
wall user sys
Min. :0.01800 Min. :0.0040 Min. :0.000
1st Qu.:0.02575 1st Qu.:0.0080 1st Qu.:0.012
Median :0.02700 Median :0.0120 Median :0.016
Mean :0.02680 Mean :0.0126 Mean :0.016
3rd Qu.:0.02925 3rd Qu.:0.0160 3rd Qu.:0.020
Max. :0.03200 Max. :0.0280 Max. :0.028
perf-stat results
------------------
Before:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
1095.617237 task-clock (msec) # 0.863 CPUs utilized ( +- 0.54% )
29,552 context-switches # 0.027 M/sec ( +- 0.75% )
354 cpu-migrations # 0.323 K/sec ( +- 4.55% )
20,848 page-faults # 0.019 M/sec ( +- 0.11% )
2,424,700,945 cycles # 2.213 GHz ( +- 0.35% )
1,369,266,675 instructions # 0.56 insn per cycle ( +- 0.12% )
286,851,480 branches # 261.817 M/sec ( +- 0.11% )
5,926,839 branch-misses # 2.07% of all branches ( +- 0.39% )
1.269172342 seconds time elapsed ( +- 0.33% )
After:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
790.618424 task-clock (msec) # 1.677 CPUs utilized ( +- 0.77% )
17,178 context-switches # 0.022 M/sec ( +- 0.91% )
3,869 cpu-migrations # 0.005 M/sec ( +- 0.76% )
20,921 page-faults # 0.026 M/sec ( +- 0.05% )
2,149,909,940 cycles # 2.719 GHz ( +- 0.41% )
1,149,982,930 instructions # 0.53 insn per cycle ( +- 0.09% )
239,776,764 branches # 303.277 M/sec ( +- 0.08% )
4,890,927 branch-misses # 2.04% of all branches ( +- 0.62% )
0.471358038 seconds time elapsed ( +- 0.76% )
Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
---
M src/kudu/client/client-test.cc
M src/kudu/util/thread-test.cc
M src/kudu/util/thread.cc
M src/kudu/util/thread.h
4 files changed, 105 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/8257/4
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] thread: improve performance of starting threads
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8257
to look at the new patch set (#3).
Change subject: thread: improve performance of starting threads
......................................................................
thread: improve performance of starting threads
This improves the performance of starting a new thread using
kudu::Thread::StartThread(). Previously, the process of starting the
thread was a little bit more complicated than necessary, involving a
sequence of the child waiting for the parent to do something, then the
parent waiting for the child to do something. Now, neither one has to
wait for the other to proceed.
The one wrinkle here is that for the parent to know the child's tid, it
does have to wait for it to publish it. However, usage of the 'tid()'
function is relatively rare, so we can defer that waiting until someone
accesses it, and then only wait in the odd case that it hasn't yet
published.
This patch includes a very simple benchmark which starts 1000 threads
and then checks the pid for each.
Timings based on the 'Time spent' log output:
---------------------------------------------
Before:
wall user sys
Min. :0.06500 Min. :0.0080 Min. :0.0160
1st Qu.:0.06975 1st Qu.:0.0160 1st Qu.:0.0200
Median :0.07100 Median :0.0200 Median :0.0240
Mean :0.07080 Mean :0.0198 Mean :0.0234
3rd Qu.:0.07200 3rd Qu.:0.0240 3rd Qu.:0.0280
Max. :0.08200 Max. :0.0280 Max. :0.0320
After:
wall user sys
Min. :0.01800 Min. :0.0040 Min. :0.000
1st Qu.:0.02575 1st Qu.:0.0080 1st Qu.:0.012
Median :0.02700 Median :0.0120 Median :0.016
Mean :0.02680 Mean :0.0126 Mean :0.016
3rd Qu.:0.02925 3rd Qu.:0.0160 3rd Qu.:0.020
Max. :0.03200 Max. :0.0280 Max. :0.028
perf-stat results
------------------
Before:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
1095.617237 task-clock (msec) # 0.863 CPUs utilized ( +- 0.54% )
29,552 context-switches # 0.027 M/sec ( +- 0.75% )
354 cpu-migrations # 0.323 K/sec ( +- 4.55% )
20,848 page-faults # 0.019 M/sec ( +- 0.11% )
2,424,700,945 cycles # 2.213 GHz ( +- 0.35% )
1,369,266,675 instructions # 0.56 insn per cycle ( +- 0.12% )
286,851,480 branches # 261.817 M/sec ( +- 0.11% )
5,926,839 branch-misses # 2.07% of all branches ( +- 0.39% )
1.269172342 seconds time elapsed ( +- 0.33% )
After:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
790.618424 task-clock (msec) # 1.677 CPUs utilized ( +- 0.77% )
17,178 context-switches # 0.022 M/sec ( +- 0.91% )
3,869 cpu-migrations # 0.005 M/sec ( +- 0.76% )
20,921 page-faults # 0.026 M/sec ( +- 0.05% )
2,149,909,940 cycles # 2.719 GHz ( +- 0.41% )
1,149,982,930 instructions # 0.53 insn per cycle ( +- 0.09% )
239,776,764 branches # 303.277 M/sec ( +- 0.08% )
4,890,927 branch-misses # 2.04% of all branches ( +- 0.62% )
0.471358038 seconds time elapsed ( +- 0.76% )
Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
---
M src/kudu/client/client-test.cc
M src/kudu/util/thread-test.cc
M src/kudu/util/thread.cc
M src/kudu/util/thread.h
4 files changed, 101 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/8257/3
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] thread: improve performance of starting threads
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8257 )
Change subject: thread: improve performance of starting threads
......................................................................
Patch Set 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/client/client-test.cc@4805
PS2, Line 4805: SleepFor(MonoDelta::FromMilliseconds(100));
Seems like the ideal value here would be different for TSAN/ASAN than for regular builds.
Regardless, I don't really understand why this test needs to start an ever increasing number of threads. Why can't it start some fixed number of threads that loop on CheckRowcount, then ASSERT_EVENTUALLY() on METRIC_rpcs_queue_overflow?
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread-test.cc
File src/kudu/util/thread-test.cc:
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread-test.cc@123
PS2, Line 123: std::vector<scoped_refptr<Thread>> threads(1000);
~Thread doesn't Join(), so shouldn't we explicitly join at the end of the test? Otherwise the detached threads will linger into the next test, no?
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@a589
PS2, Line 589:
Curious why you moved this out of here and into StartThread?
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@456
PS2, Line 456: if (Thread::current_thread() &&
: Thread::current_thread()->tid() == thread_->tid()) {
: return Status::InvalidArgument("Can't join on own thread", thread_->name_);
: }
Given tid()'s new behavior, should we demote this to a DCHECK?
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@517
PS2, Line 517: std::string Thread::ToString() const {
I presume we don't have ToString() in any hot path?
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@553
PS2, Line 553: holder->swap(old_holder_val);
Surely there's no use case for this? 'holder' is supposed to be a pure OUT parameter.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@575
PS2, Line 575: // If we failed to create the thread, we need to undo all of our prep work.
Could you use a MakeScopedCleanup for this, and cancel it on success?
It's not fewer LOC but it's a very recognizable pattern.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@591
PS2, Line 591: t->tid()
This doesn't seem like something we'd want here, given tid()'s new behavior.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@617
PS2, Line 617: thread_manager->SetThreadName(name, t->tid());
: thread_manager->AddThread(pthread_self(), name, t->category(), t->tid());
Could use tid_ here instead.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@646
PS2, Line 646: VLOG(2) << "Ended thread " << t->tid() << " - "
Could use tid_ here instead too.
http://gerrit.cloudera.org:8080/#/c/8257/2/src/kudu/util/thread.cc@648
PS2, Line 648: t->Release();
Maybe add a brief comment explaining that this could cause t to destruct, so it's important nothing accesses it afterwards.
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 11 Oct 2017 17:54:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] thread: improve performance of starting threads
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8257
to look at the new patch set (#2).
Change subject: thread: improve performance of starting threads
......................................................................
thread: improve performance of starting threads
This improves the performance of starting a new thread using
kudu::Thread::StartThread(). Previously, the process of starting the
thread was a little bit more complicated than necessary, involving a
sequence of the child waiting for the parent to do something, then the
parent waiting for the child to do something. Now, neither one has to
wait for the other to proceed.
The one wrinkle here is that for the parent to know the child's tid, it
does have to wait for it to publish it. However, usage of the 'tid()'
function is relatively rare, so we can defer that waiting until someone
accesses it, and then only wait in the odd case that it hasn't yet
published.
This patch includes a very simple benchmark which starts 1000 threads
and then checks the pid for each.
Timings based on the 'Time spent' log output:
---------------------------------------------
Before:
wall user sys
Min. :0.06500 Min. :0.0080 Min. :0.0160
1st Qu.:0.06975 1st Qu.:0.0160 1st Qu.:0.0200
Median :0.07100 Median :0.0200 Median :0.0240
Mean :0.07080 Mean :0.0198 Mean :0.0234
3rd Qu.:0.07200 3rd Qu.:0.0240 3rd Qu.:0.0280
Max. :0.08200 Max. :0.0280 Max. :0.0320
After:
wall user sys
Min. :0.01800 Min. :0.0040 Min. :0.000
1st Qu.:0.02575 1st Qu.:0.0080 1st Qu.:0.012
Median :0.02700 Median :0.0120 Median :0.016
Mean :0.02680 Mean :0.0126 Mean :0.016
3rd Qu.:0.02925 3rd Qu.:0.0160 3rd Qu.:0.020
Max. :0.03200 Max. :0.0280 Max. :0.028
perf-stat results
------------------
Before:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
1095.617237 task-clock (msec) # 0.863 CPUs utilized ( +- 0.54% )
29,552 context-switches # 0.027 M/sec ( +- 0.75% )
354 cpu-migrations # 0.323 K/sec ( +- 4.55% )
20,848 page-faults # 0.019 M/sec ( +- 0.11% )
2,424,700,945 cycles # 2.213 GHz ( +- 0.35% )
1,369,266,675 instructions # 0.56 insn per cycle ( +- 0.12% )
286,851,480 branches # 261.817 M/sec ( +- 0.11% )
5,926,839 branch-misses # 2.07% of all branches ( +- 0.39% )
1.269172342 seconds time elapsed ( +- 0.33% )
After:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
790.618424 task-clock (msec) # 1.677 CPUs utilized ( +- 0.77% )
17,178 context-switches # 0.022 M/sec ( +- 0.91% )
3,869 cpu-migrations # 0.005 M/sec ( +- 0.76% )
20,921 page-faults # 0.026 M/sec ( +- 0.05% )
2,149,909,940 cycles # 2.719 GHz ( +- 0.41% )
1,149,982,930 instructions # 0.53 insn per cycle ( +- 0.09% )
239,776,764 branches # 303.277 M/sec ( +- 0.08% )
4,890,927 branch-misses # 2.04% of all branches ( +- 0.62% )
0.471358038 seconds time elapsed ( +- 0.76% )
Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
---
M src/kudu/client/client-test.cc
M src/kudu/util/thread-test.cc
M src/kudu/util/thread.cc
M src/kudu/util/thread.h
4 files changed, 93 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/8257/2
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] thread: improve performance of starting threads
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8257 )
Change subject: thread: improve performance of starting threads
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 16:36:26 +0000
Gerrit-HasComments: No
[kudu-CR] thread: improve performance of starting threads
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8257 )
Change subject: thread: improve performance of starting threads
......................................................................
thread: improve performance of starting threads
This improves the performance of starting a new thread using
kudu::Thread::StartThread(). Previously, the process of starting the
thread was a little bit more complicated than necessary, involving a
sequence of the child waiting for the parent to do something, then the
parent waiting for the child to do something. Now, neither one has to
wait for the other to proceed.
The one wrinkle here is that for the parent to know the child's tid, it
does have to wait for it to publish it. However, usage of the 'tid()'
function is relatively rare, so we can defer that waiting until someone
accesses it, and then only wait in the odd case that it hasn't yet
published.
This patch includes a very simple benchmark which starts 1000 threads
and then checks the pid for each.
Timings based on the 'Time spent' log output:
---------------------------------------------
Before:
wall user sys
Min. :0.06500 Min. :0.0080 Min. :0.0160
1st Qu.:0.06975 1st Qu.:0.0160 1st Qu.:0.0200
Median :0.07100 Median :0.0200 Median :0.0240
Mean :0.07080 Mean :0.0198 Mean :0.0234
3rd Qu.:0.07200 3rd Qu.:0.0240 3rd Qu.:0.0280
Max. :0.08200 Max. :0.0280 Max. :0.0320
After:
wall user sys
Min. :0.01800 Min. :0.0040 Min. :0.000
1st Qu.:0.02575 1st Qu.:0.0080 1st Qu.:0.012
Median :0.02700 Median :0.0120 Median :0.016
Mean :0.02680 Mean :0.0126 Mean :0.016
3rd Qu.:0.02925 3rd Qu.:0.0160 3rd Qu.:0.020
Max. :0.03200 Max. :0.0280 Max. :0.028
perf-stat results
------------------
Before:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
1095.617237 task-clock (msec) # 0.863 CPUs utilized ( +- 0.54% )
29,552 context-switches # 0.027 M/sec ( +- 0.75% )
354 cpu-migrations # 0.323 K/sec ( +- 4.55% )
20,848 page-faults # 0.019 M/sec ( +- 0.11% )
2,424,700,945 cycles # 2.213 GHz ( +- 0.35% )
1,369,266,675 instructions # 0.56 insn per cycle ( +- 0.12% )
286,851,480 branches # 261.817 M/sec ( +- 0.11% )
5,926,839 branch-misses # 2.07% of all branches ( +- 0.39% )
1.269172342 seconds time elapsed ( +- 0.33% )
After:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
790.618424 task-clock (msec) # 1.677 CPUs utilized ( +- 0.77% )
17,178 context-switches # 0.022 M/sec ( +- 0.91% )
3,869 cpu-migrations # 0.005 M/sec ( +- 0.76% )
20,921 page-faults # 0.026 M/sec ( +- 0.05% )
2,149,909,940 cycles # 2.719 GHz ( +- 0.41% )
1,149,982,930 instructions # 0.53 insn per cycle ( +- 0.09% )
239,776,764 branches # 303.277 M/sec ( +- 0.08% )
4,890,927 branch-misses # 2.04% of all branches ( +- 0.62% )
0.471358038 seconds time elapsed ( +- 0.76% )
Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Reviewed-on: http://gerrit.cloudera.org:8080/8257
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/client/client-test.cc
M src/kudu/util/thread-test.cc
M src/kudu/util/thread.cc
M src/kudu/util/thread.h
4 files changed, 105 insertions(+), 53 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] thread: improve performance of starting threads
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, David Ribeiro Alves, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8257
to look at the new patch set (#5).
Change subject: thread: improve performance of starting threads
......................................................................
thread: improve performance of starting threads
This improves the performance of starting a new thread using
kudu::Thread::StartThread(). Previously, the process of starting the
thread was a little bit more complicated than necessary, involving a
sequence of the child waiting for the parent to do something, then the
parent waiting for the child to do something. Now, neither one has to
wait for the other to proceed.
The one wrinkle here is that for the parent to know the child's tid, it
does have to wait for it to publish it. However, usage of the 'tid()'
function is relatively rare, so we can defer that waiting until someone
accesses it, and then only wait in the odd case that it hasn't yet
published.
This patch includes a very simple benchmark which starts 1000 threads
and then checks the pid for each.
Timings based on the 'Time spent' log output:
---------------------------------------------
Before:
wall user sys
Min. :0.06500 Min. :0.0080 Min. :0.0160
1st Qu.:0.06975 1st Qu.:0.0160 1st Qu.:0.0200
Median :0.07100 Median :0.0200 Median :0.0240
Mean :0.07080 Mean :0.0198 Mean :0.0234
3rd Qu.:0.07200 3rd Qu.:0.0240 3rd Qu.:0.0280
Max. :0.08200 Max. :0.0280 Max. :0.0320
After:
wall user sys
Min. :0.01800 Min. :0.0040 Min. :0.000
1st Qu.:0.02575 1st Qu.:0.0080 1st Qu.:0.012
Median :0.02700 Median :0.0120 Median :0.016
Mean :0.02680 Mean :0.0126 Mean :0.016
3rd Qu.:0.02925 3rd Qu.:0.0160 3rd Qu.:0.020
Max. :0.03200 Max. :0.0280 Max. :0.028
perf-stat results
------------------
Before:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
1095.617237 task-clock (msec) # 0.863 CPUs utilized ( +- 0.54% )
29,552 context-switches # 0.027 M/sec ( +- 0.75% )
354 cpu-migrations # 0.323 K/sec ( +- 4.55% )
20,848 page-faults # 0.019 M/sec ( +- 0.11% )
2,424,700,945 cycles # 2.213 GHz ( +- 0.35% )
1,369,266,675 instructions # 0.56 insn per cycle ( +- 0.12% )
286,851,480 branches # 261.817 M/sec ( +- 0.11% )
5,926,839 branch-misses # 2.07% of all branches ( +- 0.39% )
1.269172342 seconds time elapsed ( +- 0.33% )
After:
Performance counter stats for 'build/latest/bin/thread-test --gtest_filter=*Benchmark* --gtest_repeat=10' (10 runs):
790.618424 task-clock (msec) # 1.677 CPUs utilized ( +- 0.77% )
17,178 context-switches # 0.022 M/sec ( +- 0.91% )
3,869 cpu-migrations # 0.005 M/sec ( +- 0.76% )
20,921 page-faults # 0.026 M/sec ( +- 0.05% )
2,149,909,940 cycles # 2.719 GHz ( +- 0.41% )
1,149,982,930 instructions # 0.53 insn per cycle ( +- 0.09% )
239,776,764 branches # 303.277 M/sec ( +- 0.08% )
4,890,927 branch-misses # 2.04% of all branches ( +- 0.62% )
0.471358038 seconds time elapsed ( +- 0.76% )
Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
---
M src/kudu/client/client-test.cc
M src/kudu/util/thread-test.cc
M src/kudu/util/thread.cc
M src/kudu/util/thread.h
4 files changed, 105 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/8257/5
--
To view, visit http://gerrit.cloudera.org:8080/8257
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5ce7f409ed33548142d1180c9f9653a8f51a7879
Gerrit-Change-Number: 8257
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>