[kudu-CR] Simplify MemTracker and move process throttling elsewhere

2017-04-26 Thread Todd Lipcon (Code Review) 
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 
---
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Simplify MemTracker and move process throttling elsewhere

2017-04-26 Thread Adar Dembo (Code Review) 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Simplify MemTracker and move process throttling elsewhere

2017-04-25 Thread Todd Lipcon (Code Review) 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Simplify MemTracker and move process throttling elsewhere

2017-04-20 Thread Adar Dembo (Code Review) 
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 1, 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 << "Total memory usage\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 
> 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Simplify MemTracker and move process throttling elsewhere

2017-04-20 Thread Todd Lipcon (Code Review) 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon