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/04/13 00:47:51 UTC
[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere
Hello David Ribeiro Alves, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6620
to review the following change.
Change subject: WIP: simplify MemTracker and move process throttling elsewhere
......................................................................
WIP: simplify MemTracker and move process throttling elsewhere
This takes a first step towards simplifying MemTracker:
- Remove the "GC function" callbacks (we never used this)
- Remove the 'ExpandLimits' code which was unimplemented.
- Remove the logging functionality, which we've never used
as far as I can remember.
- Remove soft limiting. We only used this on the root tracker, so
I just moved it to a new process_memory.{h,cc}
- Remove 'consumption_func' and un-tie the root tracker from
the global process memory usage. Now the root tracker is a simple
sum of its descendents.
Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
A src/kudu/util/process_memory.cc
A src/kudu/util/process_memory.h
12 files changed, 287 insertions(+), 434 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/6620/1
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] Simplify MemTracker and move process throttling elsewhere
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Simplify MemTracker and move process throttling elsewhere
......................................................................
Patch Set 3:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
PS3, Line 2254: since we can get accurate process memory
: // usage statistic
I presume you tested against the kNumOps of 10000, and this new value made sense?
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/server/default-path-handlers.cc
File src/kudu/server/default-path-handlers.cc:
Line 149: *output << "<h1>Total memory usage</h1>\n";
"Total" suggests that the per-subsystem stuff should add up to it. Perhaps "Process memory usage" would be more precise?
Line 152: HumanReadableNumBytes::ToString(process_memory::CurrentConsumption()));
Maybe add a warning if !TCMALLOC_ENABLED that this isn't accurate?
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/tablet/multi_column_writer.cc
File src/kudu/tablet/multi_column_writer.cc:
Line 89: LOG(INFO) << "Opened CFile writers for " << cfile_writers_.size() << " column(s)";
Heh, got tired of this output?
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/mem_tracker.cc
File src/kudu/util/mem_tracker.cc:
PS3, Line 229: // TODO: This might leave us with an allocated resource that we can't use. Do we need
: // to adjust the consumption of the query tracker to stop the resource from never
: // getting used by a subsequent TryConsume()?
Probably irrelevant to us.
Line 240: Consume(-bytes);
Technically this can fail, yet we drop the failure on the ground.
I wonder if we'd be better off just not allowing Consume(negative) and Release(negative).
Line 251: process_memory::MaybeGCAfterRelease(bytes);
Maybe this new bit should be documented in the header somewhere?
Line 264: return CheckLimitExceeded();
Could we just remove one of these two variants if they're identical?
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/mem_tracker.h
File src/kudu/util/mem_tracker.h:
PS3, Line 149: LogUsage()
Not relevant anymore. Plus, 'id' is actually used for more than just cosmetic stuff.
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
Line 20: #include <sys/resource.h>
> warning: #includes are not sorted properly [llvm-include-order]
Nit: this should be in its own group ahead of gflags/gperftools since it's a "real" system include and not a "project" system include.
Line 141: // Nothing to do if not using tcmalloc.
Maybe this should be moved up into MaybeGCAfterRelease() so we can avoid the increment on g_released_memory_since_gc?
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: Yes
[kudu-CR] Simplify MemTracker and move process throttling elsewhere
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6620
to look at the new patch set (#4).
Change subject: Simplify MemTracker and move process throttling elsewhere
......................................................................
Simplify MemTracker and move process throttling elsewhere
This takes a first step towards simplifying MemTracker:
- Remove the "GC function" callbacks (we never used this)
- Remove the 'ExpandLimits' code which was unimplemented.
- Remove the logging functionality, which we've never used
as far as I can remember.
- Remove soft limiting. We only used this on the root tracker, so
I just moved it to a new process_memory.{h,cc}
- Remove 'consumption_func' and un-tie the root tracker from
the global process memory usage. Now the root tracker is a simple
sum of its descendents.
For a stress/benchmark I ran a 500GB YCSB with a memory limit set to
10GB. Results showed no major difference with this patch (throughput was
a few percent faster but within the realm of noise). Details at [1]
[1] https://docs.google.com/document/d/1dOe5-L5BWUhF-uV4-AE5hduvfUWctXizlaoSLlp38yM/edit?usp=sharing
Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
A src/kudu/util/process_memory.cc
A src/kudu/util/process_memory.h
14 files changed, 373 insertions(+), 613 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/6620/4
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
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] Simplify MemTracker and move process throttling elsewhere
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Simplify MemTracker and move process throttling elsewhere
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/mem_tracker.cc
File src/kudu/util/mem_tracker.cc:
Line 240: if (bytes == 0) {
> I don't think Consume() can fail (only TryConsume fails). Consume always su
Ah yes, my bad. Was looking at TryConsume() when I wrote that comment.
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: Yes
[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP: simplify MemTracker and move process throttling elsewhere
......................................................................
Patch Set 1:
Yea, I agree that all three of those things may disappear. I just wanted to do some smaller patch to start to get rid of some of the cruft that we weren't using, and to move out the throttling stuff to a new file (since that stuff will remain). Then when we fully remove the existing code it will be more obvious what was actually removed.
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Simplify MemTracker and move process throttling elsewhere
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Simplify MemTracker and move process throttling elsewhere
......................................................................
Simplify MemTracker and move process throttling elsewhere
This takes a first step towards simplifying MemTracker:
- Remove the "GC function" callbacks (we never used this)
- Remove the 'ExpandLimits' code which was unimplemented.
- Remove the logging functionality, which we've never used
as far as I can remember.
- Remove soft limiting. We only used this on the root tracker, so
I just moved it to a new process_memory.{h,cc}
- Remove 'consumption_func' and un-tie the root tracker from
the global process memory usage. Now the root tracker is a simple
sum of its descendents.
For a stress/benchmark I ran a 500GB YCSB with a memory limit set to
10GB. Results showed no major difference with this patch (throughput was
a few percent faster but within the realm of noise). Details at [1]
[1] https://docs.google.com/document/d/1dOe5-L5BWUhF-uV4-AE5hduvfUWctXizlaoSLlp38yM/edit?usp=sharing
Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Reviewed-on: http://gerrit.cloudera.org:8080/6620
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
A src/kudu/util/process_memory.cc
A src/kudu/util/process_memory.h
14 files changed, 373 insertions(+), 613 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
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] WIP: simplify MemTracker and move process throttling elsewhere
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6620
to look at the new patch set (#2).
Change subject: WIP: simplify MemTracker and move process throttling elsewhere
......................................................................
WIP: simplify MemTracker and move process throttling elsewhere
This takes a first step towards simplifying MemTracker:
- Remove the "GC function" callbacks (we never used this)
- Remove the 'ExpandLimits' code which was unimplemented.
- Remove the logging functionality, which we've never used
as far as I can remember.
- Remove soft limiting. We only used this on the root tracker, so
I just moved it to a new process_memory.{h,cc}
- Remove 'consumption_func' and un-tie the root tracker from
the global process memory usage. Now the root tracker is a simple
sum of its descendents.
Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
A src/kudu/util/process_memory.cc
A src/kudu/util/process_memory.h
12 files changed, 298 insertions(+), 434 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/6620/2
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Simplify MemTracker and move process throttling elsewhere
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Simplify MemTracker and move process throttling elsewhere
......................................................................
Patch Set 3:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
PS3, Line 2254: since we can get accurate process memory
: // usage statistic
> I presume you tested against the kNumOps of 10000, and this new value made
yea this magic number seemed to pass
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/server/default-path-handlers.cc
File src/kudu/server/default-path-handlers.cc:
Line 44: #include "kudu/util/process_memory.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
Line 47: using boost::replace_all;
> warning: using decl 'replace_all' is unused [misc-unused-using-decls]
Done
Line 148: static void MemTrackersHandler(const Webserver::WebRequest& req, std::ostringstream* output) {
> warning: parameter 'req' is unused [misc-unused-parameters]
Done
Line 149: *output << "<h1>Total memory usage</h1>\n";
> "Total" suggests that the per-subsystem stuff should add up to it. Perhaps
Done
Line 152: HumanReadableNumBytes::ToString(process_memory::CurrentConsumption()));
> Maybe add a warning if !TCMALLOC_ENABLED that this isn't accurate?
Done
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/tablet/multi_column_writer.cc
File src/kudu/tablet/multi_column_writer.cc:
Line 89: LOG(INFO) << "Opened CFile writers for " << cfile_writers_.size() << " column(s)";
> Heh, got tired of this output?
yep :)
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/mem_tracker.cc
File src/kudu/util/mem_tracker.cc:
PS3, Line 229: // TODO: This might leave us with an allocated resource that we can't use. Do we need
: // to adjust the consumption of the query tracker to stop the resource from never
: // getting used by a subsequent TryConsume()?
> Probably irrelevant to us.
Done
Line 240: Consume(-bytes);
> Technically this can fail, yet we drop the failure on the ground.
I don't think Consume() can fail (only TryConsume fails). Consume always succeeds, right?
Line 251: process_memory::MaybeGCAfterRelease(bytes);
> Maybe this new bit should be documented in the header somewhere?
it's not new, but added it to the header.
Line 264: return CheckLimitExceeded();
> Could we just remove one of these two variants if they're identical?
Done
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/mem_tracker.h
File src/kudu/util/mem_tracker.h:
PS3, Line 149: LogUsage()
> Not relevant anymore. Plus, 'id' is actually used for more than just cosmet
Done
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
Line 20: #include <sys/resource.h>
> Nit: this should be in its own group ahead of gflags/gperftools since it's
Done
Line 76: static const int64_t GC_RELEASE_SIZE = 128 * 1024L * 1024L;
> warning: 'GC_RELEASE_SIZE' is a static definition in anonymous namespace; s
Done
Line 141: // Nothing to do if not using tcmalloc.
> Maybe this should be moved up into MaybeGCAfterRelease() so we can avoid th
Done
http://gerrit.cloudera.org:8080/#/c/6620/3/src/kudu/util/process_memory.h
File src/kudu/util/process_memory.h:
Line 30: void MaybeGCAfterRelease(int64_t released_bytes);
> error: unknown type name 'int64_t' [clang-diagnostic-error]
Done
Line 33: int64_t CurrentConsumption();
> error: unknown type name 'int64_t' [clang-diagnostic-error]
Done
Line 36: int64_t HardLimit();
> error: unknown type name 'int64_t' [clang-diagnostic-error]
Done
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: Yes
[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: WIP: simplify MemTracker and move process throttling elsewhere
......................................................................
Patch Set 2:
> do you have any reservations about this particular patch?
> Or just suggesting it might be easier to start clean?
No reservations, just thought it'd be net simpler to start clean.
> If you aren't strongly against, I'd like to continue down this
> route.
Sure, works for me. I'll review this in depth once you're comfortable that it's no longer WIP.
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: WIP: simplify MemTracker and move process throttling elsewhere
......................................................................
Patch Set 1:
Not sure exactly what your plan is, so I'll just list the things that I found surprising:
1. Why preserve the hierarchical nature of MemTracker? Based on our in-person discussion yesterday, I thought all of the things worth tracking are isolated from another.
2. Why preserve the root tracker?
3. Do MemTrackers still need shared ownership? If so, why?
If you agree that the above three things can be removed, I think you'll end up removing more than is actually left behind; that is, it may be simpler to reimplement based on the new set of requirements than it is to pare the existing MemTracker down.
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] WIP: simplify MemTracker and move process throttling elsewhere
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP: simplify MemTracker and move process throttling elsewhere
......................................................................
Patch Set 2:
Sounds good, I'll try to push this to commitable status in the next day or two including running a stress workload to make sure the actual memory usage stays within bounds, perf didn't regress, etc.
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Simplify MemTracker and move process throttling elsewhere
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6620
to look at the new patch set (#3).
Change subject: Simplify MemTracker and move process throttling elsewhere
......................................................................
Simplify MemTracker and move process throttling elsewhere
This takes a first step towards simplifying MemTracker:
- Remove the "GC function" callbacks (we never used this)
- Remove the 'ExpandLimits' code which was unimplemented.
- Remove the logging functionality, which we've never used
as far as I can remember.
- Remove soft limiting. We only used this on the root tracker, so
I just moved it to a new process_memory.{h,cc}
- Remove 'consumption_func' and un-tie the root tracker from
the global process memory usage. Now the root tracker is a simple
sum of its descendents.
For a stress/benchmark I ran a 500GB YCSB with a memory limit set to
10GB. Results showed no major difference with this patch (throughput was
a few percent faster but within the realm of noise). Details at [1]
[1] https://docs.google.com/document/d/1dOe5-L5BWUhF-uV4-AE5hduvfUWctXizlaoSLlp38yM/edit?usp=sharing
Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
M src/kudu/util/mem_tracker-test.cc
M src/kudu/util/mem_tracker.cc
M src/kudu/util/mem_tracker.h
A src/kudu/util/process_memory.cc
A src/kudu/util/process_memory.h
14 files changed, 349 insertions(+), 594 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/6620/3
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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] WIP: simplify MemTracker and move process throttling elsewhere
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP: simplify MemTracker and move process throttling elsewhere
......................................................................
Patch Set 2:
Adar -- do you have any reservations about this particular patch? Or just suggesting it might be easier to start clean?
If you aren't strongly against, I'd like to continue down this route. Once this patch is committed, we'll have better separation between MemTracker and throttling logic, and can make some progress on improving the throttling vs flush-triggering issues discussed in https://docs.google.com/document/d/17-2CcmrjxZY0Gd9wDUh83xCCNL574famw8_2Bhfu-_8/edit
The missing piece that makes this a WIP is periodically triggering the tcmalloc ReleaseMemory() call as necessary, after which point I think it should be committable.
--
To view, visit http://gerrit.cloudera.org:8080/6620
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id16bad7d9a29a83e820a38e9d703811391cffe90
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No