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 2016/12/15 10:36:15 UTC
[kudu-CR] threadpool: avoid calling task destructors while holding lock
Hello Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5517
to review the following change.
Change subject: threadpool: avoid calling task destructors while holding lock
......................................................................
threadpool: avoid calling task destructors while holding lock
Currently, the ThreadPool calls task's destructors while holding its
internal lock. This has two issues:
1) While it's not a great practice, some destructors themselves acquire
locks. This can create lock cycles if those same locks are ever held
concurrently with a submission to the threadpool.
2) Even well-written destructors can be heavy-weight, eg if they free a
large graph of dependent objects. We should not block the threadpool's
progress while running these destructors.
This fixes the issue by being more explicit about where tasks are freed,
both in the Shutdown() code path as well as the normal task dispatch
code.
Change-Id: I4cdc38c7db0a6a7fd640d82895ecaebe0718ee98
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 48 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/5517/1
--
To view, visit http://gerrit.cloudera.org:8080/5517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4cdc38c7db0a6a7fd640d82895ecaebe0718ee98
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] threadpool: avoid calling task destructors while holding lock
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool: avoid calling task destructors while holding lock
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5517/1/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
Line 369: ASSERT_OK(thread_pool->Submit(std::move(task)));
> warning: passing result of std::move() as a const reference argument; no mo
fixed in a separate commit which makes the threadpool support move semantics
--
To view, visit http://gerrit.cloudera.org:8080/5517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cdc38c7db0a6a7fd640d82895ecaebe0718ee98
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] threadpool: avoid calling task destructors while holding lock
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool: avoid calling task destructors while holding lock
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5517/1/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
Line 171: unique_lock.Unlock();
> Not a big deal, but why not use an RAII end-of-scope to destroy/unlock this
considered it, but decided not to since then I would have had to move the 'to_release' definition outside of that scope, yielding more lines of code and not being able to use 'auto'
--
To view, visit http://gerrit.cloudera.org:8080/5517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cdc38c7db0a6a7fd640d82895ecaebe0718ee98
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] threadpool: avoid calling task destructors while holding lock
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: threadpool: avoid calling task destructors while holding lock
......................................................................
threadpool: avoid calling task destructors while holding lock
Currently, the ThreadPool calls task's destructors while holding its
internal lock. This has two issues:
1) While it's not a great practice, some destructors themselves acquire
locks. This can create lock cycles if those same locks are ever held
concurrently with a submission to the threadpool.
2) Even well-written destructors can be heavy-weight, eg if they free a
large graph of dependent objects. We should not block the threadpool's
progress while running these destructors.
This fixes the issue by being more explicit about where tasks are freed,
both in the Shutdown() code path as well as the normal task dispatch
code.
Change-Id: I4cdc38c7db0a6a7fd640d82895ecaebe0718ee98
Reviewed-on: http://gerrit.cloudera.org:8080/5517
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
3 files changed, 48 insertions(+), 15 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4cdc38c7db0a6a7fd640d82895ecaebe0718ee98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] threadpool: avoid calling task destructors while holding lock
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: threadpool: avoid calling task destructors while holding lock
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5517/1/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
Line 171: unique_lock.Unlock();
Not a big deal, but why not use an RAII end-of-scope to destroy/unlock this unique_lock?
--
To view, visit http://gerrit.cloudera.org:8080/5517
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4cdc38c7db0a6a7fd640d82895ecaebe0718ee98
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes