Add option to disable measuring buffer copy #11996
Conversation
Signed-off-by: Ray Liu <[email protected]>
Signed-off-by: Ray Liu <[email protected]>
liurenjie1024
force-pushed
the
ray/option_to_disable_buffer_copy
branch
from
6bdaa67 to
5a27ca3
Compare
Signed-off-by: Ray Liu <[email protected]>
Signed-off-by: Ray Liu <[email protected]>
|
build |
| @@ -2025,6 +2025,15 @@ val SHUFFLE_COMPRESSION_LZ4_CHUNK_SIZE = conf("spark.rapids.shuffle.compression. | |||
| .booleanConf | |||
| .createWithDefault(false) | |||
|
|
|||
| val SHUFFLE_KUDO_SERIALIZER_MEASURE_BUFFER_COPY_ENABLED = | |||
There was a problem hiding this comment.
Is there any case we still need measuring buffer on for kudo? If not, should consider removing it.
There was a problem hiding this comment.
I want to keep it to do some experiments.
| @@ -172,6 +172,9 @@ abstract class GpuShuffleExchangeExecBase( | |||
| import GpuMetric._ | |||
|
|
|||
| private lazy val useKudo = RapidsConf.SHUFFLE_KUDO_SERIALIZER_ENABLED.get(child.conf) | |||
| private lazy val kudoMeasureBufferCopy = RapidsConf | |||
There was a problem hiding this comment.
Could we please consider another name for this variable? I quite like the convention used for the conf key:
| private lazy val kudoMeasureBufferCopy = RapidsConf | |
| private lazy val kudoBufferCopyMeasurementEnabled = RapidsConf |
There was a problem hiding this comment.
Cool, will do this in next pr to unblock merging.
There was a problem hiding this comment.
Fixed in #12081
There was a problem hiding this comment.
I am concerned and confused by this patch. It appears that this patch does not actually reduce the copy time overhead at all. It just takes a step and adds a config to stop collecting a metric. But it does not disabling that metric when it happens.
So if someone enables debug metrics and kudo then these are not going to show up at all (by default), but it will still display a number saying that it is 0. This is also not documented anywhere so if someone did look, which I doubt that they would, they would not find much of anything and just assume that the value is actually 0.
Can we at least have a follow on issue to fix this? At a minimum document it, ideally if the config is disabled we disable the metric also.
|
Hi, @revans2
To fully enable/disable measuring buffer copy time, I've split the whole issue to three parts, see #11995 for more details, and this pr is just the first step. Second pr is here: NVIDIA/spark-rapids-jni#2759.
It's not. Even if we created the metrics, as long as we didn't do any modification to this metric, it still will not show up in ui. I did experiments in local and verified it. |
sameerz
added
the
task
This is the first step of #11995 .
It adds an option to disable measuring copy buffer time in spark-rapids. It doesn't actually disable it for kudo serializer, but could hide metrics.