You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2020/02/20 17:14:12 UTC
[kudu-CR] KUDU-3056: Reduce HdrHistogramAccumulator overhead
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15254
Change subject: KUDU-3056: Reduce HdrHistogramAccumulator overhead
......................................................................
KUDU-3056: Reduce HdrHistogramAccumulator overhead
This patch makes a few changes to reduce the overhead of the
HdrHistogramAccumulator.
It changes from using `SynchronizedHistogram` (value type
long) to using `IntCountsHistogram` (value type int).
This significantly reduces the data footprint of the histogram and is
safe given write durations will never exceed `Integer.MAX_VALUE`.
Because thread safety is still important we syncronize all access
to `IntCountsHistogram` in `HistogramWrapper`.
It also adjust the `HistogramWrapper` to lazily instantiate an
`IntCountsHistogram`. This means that if no values are recorded,
the overhead of the `HdrHistogramAccumulator` should be almost
zero.
Last it reduces the `numberOfSignificantValueDigits` tracked
in the histogram from 3 to 2. The result is relatively similar
output in the Spark accumulator with a significantly smaller
histogram.
I tested each variant using `getEstimatedFootprintInBytes()` and
the result is that the new implimentation is 90% smaller when the
HdrHistogramAccumulator is used. The new implementation
is 100% smaller when not no values are stored:
long w/ precision 3 & max 30000ms: 49664 (current)
long w/ precision 2 & max 30000ms: 9728
long w/ precision 1 & max 30000ms: 2048
int w/ precision 3 & max 30000ms: 25088
int w/ precision 2 & max 30000ms: 5120 (new)
int w/ precision 1 & max 30000ms: 1280
Note: I used a max of 30000ms in these calculations because that
is the default operation timeout
Below is sample string output from before and after this patch
generated with 1000 random values between 0ms and 500ms.
Before:
0.2%: 0ms, 50.3%: 265ms, 75.1%: 376ms, 87.5%: 437ms, 93.8%: 470ms, 96.9%: 484ms, 98.6%: 493ms, 99.5%: 496ms, 99.8%: 498ms, 100.0%: 499ms, 100.0%: 499ms
After:
0.2%: 0ms, 50.3%: 265ms, 75.4%: 377ms, 87.5%: 437ms, 93.9%: 471ms, 97.3%: 485ms, 98.6%: 493ms, 99.5%: 497ms, 100.0%: 499ms, 100.0%: 499ms
Note: I used the same seed to generate the same values for both strings.
Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 68 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/15254/1
--
To view, visit http://gerrit.cloudera.org:8080/15254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Gerrit-Change-Number: 15254
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] KUDU-3056: Reduce HdrHistogramAccumulator overhead
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-3056: Reduce HdrHistogramAccumulator overhead
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Gerrit-Change-Number: 15254
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3056: Reduce HdrHistogramAccumulator overhead
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15254 )
Change subject: KUDU-3056: Reduce HdrHistogramAccumulator overhead
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@19
PS1, Line 19: adjust
adjusts
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@24
PS1, Line 24: Last
Lastly
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@25
PS1, Line 25: The result is relatively similar
: output in the Spark accumulator with a significantly smaller
: histogram.
I took a quick look at the HdrHistograms we use server-side. Most use a num_sig_digits of 2, and those that use 3 are server-level (rather than tablet-level) so that the increase in memory footprint doesn't scale with server load.
For context, here's what Will said about the initial HdrHistogramAccumulator sizing:
HdrHistograms need to be shipped between executors and the driver, so
their (serialized) size is relevant. Spark users differ in how they
serialize, so I didn't put much effort into estimating the serialized
size, but based on the conservative formula in [1] the in-memory size of
a histogram with 3 significant value digits and storing longs is 4MiB or
so. That only happens if the histogram is storing values from 1 to the
max trackable long value, which is Long.MAX / 2. More realistically,
the values in the duration histogram should be at most 86400 * 1000, the
number of milliseconds in a day, and usually much, much smaller. For
that range of values, the max footprint is 1MiB. That should be a safe
amount of data to ship about semi-frequently along with all the Kudu
data (and I'm not counting potential compression as part of
serialization).
[1]: https://github.com/HdrHistogram/HdrHistogram#footprint-estimation
Sounds like maybe he didn't factor in the number of executors that may be in use?
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@30
PS1, Line 30: implimentation
implementation
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@32
PS1, Line 32: not
drop this
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@53
PS1, Line 53: to
"so as to"
http://gerrit.cloudera.org:8080/#/c/15254/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala
File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala:
http://gerrit.cloudera.org:8080/#/c/15254/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala@47
PS1, Line 47: histogram
Why don't we need .copy() here any more?
--
To view, visit http://gerrit.cloudera.org:8080/15254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Gerrit-Change-Number: 15254
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 Feb 2020 17:42:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3056: Reduce HdrHistogramAccumulator overhead
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15254 )
Change subject: KUDU-3056: Reduce HdrHistogramAccumulator overhead
......................................................................
KUDU-3056: Reduce HdrHistogramAccumulator overhead
This patch makes a few changes to reduce the overhead of the
HdrHistogramAccumulator.
It changes from using `SynchronizedHistogram` (value type
long) to using `IntCountsHistogram` (value type int).
This significantly reduces the data footprint of the histogram and is
safe given write durations will never exceed `Integer.MAX_VALUE`.
Because thread safety is still important we syncronize all access
to `IntCountsHistogram` in `HistogramWrapper`.
It also adjusts the `HistogramWrapper` to lazily instantiate an
`IntCountsHistogram`. This means that if no values are recorded,
the overhead of the `HdrHistogramAccumulator` should be almost
zero.
Lastly it reduces the `numberOfSignificantValueDigits` tracked
in the histogram from 3 to 2. The result is relatively similar
output in the Spark accumulator with a significantly smaller
histogram.
I tested each variant using `getEstimatedFootprintInBytes()` and
the result is that the new implementation is 90% smaller when the
HdrHistogramAccumulator is used. The new implementation
is 100% smaller when no values are stored:
long w/ precision 3 & max 30000ms: 49664 (current)
long w/ precision 2 & max 30000ms: 9728
long w/ precision 1 & max 30000ms: 2048
int w/ precision 3 & max 30000ms: 25088
int w/ precision 2 & max 30000ms: 5120 (new)
int w/ precision 1 & max 30000ms: 1280
Note: I used a max of 30000ms in these calculations because that
is the default operation timeout
Below is sample string output from before and after this patch
generated with 1000 random values between 0ms and 500ms.
Before:
0.2%: 0ms, 50.3%: 265ms, 75.1%: 376ms, 87.5%: 437ms, 93.8%: 470ms, 96.9%: 484ms, 98.6%: 493ms, 99.5%: 496ms, 99.8%: 498ms, 100.0%: 499ms, 100.0%: 499ms
After:
0.2%: 0ms, 50.3%: 265ms, 75.4%: 377ms, 87.5%: 437ms, 93.9%: 471ms, 97.3%: 485ms, 98.6%: 493ms, 99.5%: 497ms, 100.0%: 499ms, 100.0%: 499ms
Note: I used the same seed so as to generate the same values for both strings.
Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Reviewed-on: http://gerrit.cloudera.org:8080/15254
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Grant Henke <gr...@apache.org>
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 72 insertions(+), 27 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Grant Henke: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Gerrit-Change-Number: 15254
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3056: Reduce HdrHistogramAccumulator overhead
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15254 )
Change subject: KUDU-3056: Reduce HdrHistogramAccumulator overhead
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Gerrit-Change-Number: 15254
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 Feb 2020 19:11:09 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3056: Reduce HdrHistogramAccumulator overhead
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15254
to look at the new patch set (#2).
Change subject: KUDU-3056: Reduce HdrHistogramAccumulator overhead
......................................................................
KUDU-3056: Reduce HdrHistogramAccumulator overhead
This patch makes a few changes to reduce the overhead of the
HdrHistogramAccumulator.
It changes from using `SynchronizedHistogram` (value type
long) to using `IntCountsHistogram` (value type int).
This significantly reduces the data footprint of the histogram and is
safe given write durations will never exceed `Integer.MAX_VALUE`.
Because thread safety is still important we syncronize all access
to `IntCountsHistogram` in `HistogramWrapper`.
It also adjusts the `HistogramWrapper` to lazily instantiate an
`IntCountsHistogram`. This means that if no values are recorded,
the overhead of the `HdrHistogramAccumulator` should be almost
zero.
Lastly it reduces the `numberOfSignificantValueDigits` tracked
in the histogram from 3 to 2. The result is relatively similar
output in the Spark accumulator with a significantly smaller
histogram.
I tested each variant using `getEstimatedFootprintInBytes()` and
the result is that the new implementation is 90% smaller when the
HdrHistogramAccumulator is used. The new implementation
is 100% smaller when no values are stored:
long w/ precision 3 & max 30000ms: 49664 (current)
long w/ precision 2 & max 30000ms: 9728
long w/ precision 1 & max 30000ms: 2048
int w/ precision 3 & max 30000ms: 25088
int w/ precision 2 & max 30000ms: 5120 (new)
int w/ precision 1 & max 30000ms: 1280
Note: I used a max of 30000ms in these calculations because that
is the default operation timeout
Below is sample string output from before and after this patch
generated with 1000 random values between 0ms and 500ms.
Before:
0.2%: 0ms, 50.3%: 265ms, 75.1%: 376ms, 87.5%: 437ms, 93.8%: 470ms, 96.9%: 484ms, 98.6%: 493ms, 99.5%: 496ms, 99.8%: 498ms, 100.0%: 499ms, 100.0%: 499ms
After:
0.2%: 0ms, 50.3%: 265ms, 75.4%: 377ms, 87.5%: 437ms, 93.9%: 471ms, 97.3%: 485ms, 98.6%: 493ms, 99.5%: 497ms, 100.0%: 499ms, 100.0%: 499ms
Note: I used the same seed so as to generate the same values for both strings.
Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
2 files changed, 72 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/15254/2
--
To view, visit http://gerrit.cloudera.org:8080/15254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Gerrit-Change-Number: 15254
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3056: Reduce HdrHistogramAccumulator overhead
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15254 )
Change subject: KUDU-3056: Reduce HdrHistogramAccumulator overhead
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@19
PS1, Line 19: adjust
> adjusts
Done
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@24
PS1, Line 24: Last
> Lastly
Done
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@25
PS1, Line 25: The result is relatively similar
: output in the Spark accumulator with a significantly smaller
: histogram.
> I took a quick look at the HdrHistograms we use server-side. Most use a num
It's good to know this aligns with the server side significant digits.
Yeah, his claim is accurate. But the total transmitted size back to the spark driver is num-tasks * histogram-size. So 1 MiB is a lot for the driver to collect when there are hundreds of tasks. (In the reported Jira there are ~900).
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@30
PS1, Line 30: implimentation
> implementation
Done
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@32
PS1, Line 32: not
> drop this
Done
http://gerrit.cloudera.org:8080/#/c/15254/1//COMMIT_MSG@53
PS1, Line 53: to
> "so as to"
Done
http://gerrit.cloudera.org:8080/#/c/15254/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala
File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala:
http://gerrit.cloudera.org:8080/#/c/15254/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/HdrHistogramAccumulator.scala@47
PS1, Line 47: histogram
> Why don't we need .copy() here any more?
ooh, good catch.
--
To view, visit http://gerrit.cloudera.org:8080/15254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Gerrit-Change-Number: 15254
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 Feb 2020 18:30:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3056: Reduce HdrHistogramAccumulator overhead
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15254 )
Change subject: KUDU-3056: Reduce HdrHistogramAccumulator overhead
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15254
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7c2a33bc61a2baa38703ea3340a07e06ab39db3
Gerrit-Change-Number: 15254
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 20 Feb 2020 19:43:32 +0000
Gerrit-HasComments: No