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/05/18 00:26:22 UTC
[kudu-CR] process memory: go back to non-incremental tracking
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6915
to review the following change.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
process_memory: go back to non-incremental tracking
The incremental tracking has more overhead than expected, even with our
fancy striped counters, etc. In tpch_real_world benchmarks, the
LongAdder::IncrementBy() and GetAllocatedSize() calls in the hooks
were taking a significant amount of CPU (in the top 10 consumers).
Here's yet another approach: back to non-incremental tracking, but with
an optimization that, if we've calculated the memory usage in the last
50ms, or if another thread is already in the process of calculating, we
don't re-calculate it.
This has some minor risk of "overshooting" the memory limit, but since
our limiting is already probabilistic and not 100% thorough anyway, I
think that's acceptable.
Benchmarked using process_memory-test:
Before:
I0517 16:46:09.596946 7419 process_memory-test.cc:68] Performed 2.18441e+06 iters/sec
After:
I0517 16:45:50.497973 7051 process_memory-test.cc:68] Performed 1.22264e+08 iters/sec
(55x speedup)
I also ran tpch_real_world and graphed CPU consumed per inserted row and
found it to be noticeably better with this optimized version. I
spot-checked perf results and don't see any tcmalloc stat-related
methods in the high consumers list anymore. I also checked in this
workload that the heap usage was properly limited (indistinguishable
from the before-patch implementation)
Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
---
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/util/process_memory.cc
2 files changed, 23 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/6915/1
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] process memory: go back to non-incremental tracking
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6915/1//COMMIT_MSG
Commit Message:
PS1, Line 34: noticeably better
> Thanks. It looks like the difference between the two implementations is abo
I think it's more significant -- eg, where the red line crosses 20 us/row, the blue line is around 18.5 us/row. So, that's 7.5% or so.
http://gerrit.cloudera.org:8080/#/c/6915/1/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
Line 191: const int64_t kReadIntervalMicros = 50000;
> I thought we could consume more than that, even in such a short period of t
If we're doing 1M inserts/sec, then it must be with a smaller row size (1M * 1k row size = 1GB/sec, which is more than I've seen us ingest per node). And I think in that case, overshooting by 50M isn't so bad, either, considering to sustain 1GB ingest per second you must have a relatively high memory limit, lots of maintenance threads, etc, and thus 50M is only a tiny percentage of your overall heap, right?
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] process memory: go back to non-incremental tracking
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6915/1//COMMIT_MSG
Commit Message:
PS1, Line 34: noticeably better
> takes a long time to run, so I didn't wait for the whole thing to finish :)
Thanks. It looks like the difference between the two implementations is about 1-2% CPU time. Am I reading the graph correctly?
http://gerrit.cloudera.org:8080/#/c/6915/1/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
Line 191: const int64_t kReadIntervalMicros = 50000;
> figured that we would have a hard time using more than a few MB in 50ms, an
I thought we could consume more than that, even in such a short period of time. Like, ballpark 1M inserts/s, so 50K for every 50ms. With a 1K row size, that's 50 MB.
Do you think that's achievable?
PS1, Line 198: GetMonoTimeMicros()
> I wanted to re-fetch time, so that we guarantee the sleep _between_ calls i
Makes sense. Could you add a comment explaining this, so no one "optimizes away" the second call?
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] process memory: go back to non-incremental tracking
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6915/1//COMMIT_MSG
Commit Message:
PS1, Line 34: noticeably better
> Can you quantify that?
takes a long time to run, so I didn't wait for the whole thing to finish :) Here's the plot I looked at, though:
https://imagebin.ca/v/3Mq0VwvSGSJ1
http://gerrit.cloudera.org:8080/#/c/6915/1/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
Line 191: const int64_t kReadIntervalMicros = 50000;
> I'm curious as to how you arrived at this number. My instinct would be to g
figured that we would have a hard time using more than a few MB in 50ms, and it would definitely avoid contention?
PS1, Line 198: GetMonoTimeMicros()
> Shouldn't this be 'time'?
I wanted to re-fetch time, so that we guarantee the sleep _between_ calls is at least 50ms. That way, if the call itself started being slow (eg because we have 10000 threads) we guarantee some progress
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] process memory: go back to non-incremental tracking
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6915/1//COMMIT_MSG
Commit Message:
PS1, Line 34: noticeably better
Can you quantify that?
http://gerrit.cloudera.org:8080/#/c/6915/1/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
Line 191: const int64_t kReadIntervalMicros = 50000;
I'm curious as to how you arrived at this number. My instinct would be to go lower, like 10ms.
PS1, Line 198: GetMonoTimeMicros()
Shouldn't this be 'time'?
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] process memory: go back to non-incremental tracking
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/6915
to look at the new patch set (#2).
Change subject: process_memory: go back to non-incremental tracking
......................................................................
process_memory: go back to non-incremental tracking
The incremental tracking has more overhead than expected, even with our
fancy striped counters, etc. In tpch_real_world benchmarks, the
LongAdder::IncrementBy() and GetAllocatedSize() calls in the hooks
were taking a significant amount of CPU (in the top 10 consumers).
Here's yet another approach: back to non-incremental tracking, but with
an optimization that, if we've calculated the memory usage in the last
50ms, or if another thread is already in the process of calculating, we
don't re-calculate it.
This has some minor risk of "overshooting" the memory limit, but since
our limiting is already probabilistic and not 100% thorough anyway, I
think that's acceptable.
Benchmarked using process_memory-test:
Before:
I0517 16:46:09.596946 7419 process_memory-test.cc:68] Performed 2.18441e+06 iters/sec
After:
I0517 16:45:50.497973 7051 process_memory-test.cc:68] Performed 1.22264e+08 iters/sec
(55x speedup)
I also ran tpch_real_world and graphed CPU consumed per inserted row and
found it to be noticeably better with this optimized version
(approximately 7-8% by eye-balling it). I spot-checked perf results and
don't see any tcmalloc stat-related methods in the high consumers list
anymore. I also checked in this workload that the heap usage was
properly limited (indistinguishable from the before-patch
implementation)
Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
---
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/util/process_memory.cc
2 files changed, 28 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/6915/2
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] process memory: go back to non-incremental tracking
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] process memory: go back to non-incremental tracking
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6915/1//COMMIT_MSG
Commit Message:
PS1, Line 34: noticeably better
> I think it's more significant -- eg, where the red line crosses 20 us/row,
Ah yes, the Y axis is raw cpu micros, not percentages. Makes sense, thanks for the correction.
http://gerrit.cloudera.org:8080/#/c/6915/1/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
Line 191: const int64_t kReadIntervalMicros = 50000;
> If we're doing 1M inserts/sec, then it must be with a smaller row size (1M
I buy it.
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] process memory: go back to non-incremental tracking
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
process_memory: go back to non-incremental tracking
The incremental tracking has more overhead than expected, even with our
fancy striped counters, etc. In tpch_real_world benchmarks, the
LongAdder::IncrementBy() and GetAllocatedSize() calls in the hooks
were taking a significant amount of CPU (in the top 10 consumers).
Here's yet another approach: back to non-incremental tracking, but with
an optimization that, if we've calculated the memory usage in the last
50ms, or if another thread is already in the process of calculating, we
don't re-calculate it.
This has some minor risk of "overshooting" the memory limit, but since
our limiting is already probabilistic and not 100% thorough anyway, I
think that's acceptable.
Benchmarked using process_memory-test:
Before:
I0517 16:46:09.596946 7419 process_memory-test.cc:68] Performed 2.18441e+06 iters/sec
After:
I0517 16:45:50.497973 7051 process_memory-test.cc:68] Performed 1.22264e+08 iters/sec
(55x speedup)
I also ran tpch_real_world and graphed CPU consumed per inserted row and
found it to be noticeably better with this optimized version
(approximately 7-8% by eye-balling it). I spot-checked perf results and
don't see any tcmalloc stat-related methods in the high consumers list
anymore. I also checked in this workload that the heap usage was
properly limited (indistinguishable from the before-patch
implementation)
Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
Reviewed-on: http://gerrit.cloudera.org:8080/6915
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/tserver/tablet_server-stress-test.cc
M src/kudu/util/process_memory.cc
2 files changed, 28 insertions(+), 41 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] process memory: go back to non-incremental tracking
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: process_memory: go back to non-incremental tracking
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6915/1/src/kudu/util/process_memory.cc
File src/kudu/util/process_memory.cc:
PS1, Line 198: GetMonoTimeMicros()
> Makes sense. Could you add a comment explaining this, so no one "optimizes
Done
--
To view, visit http://gerrit.cloudera.org:8080/6915
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8823028de3ea260f1450d9bf34af2dc5a794b206
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes