Copilot commented on code in PR #12528:
URL: https://github.com/apache/gluten/pull/12528#discussion_r3612297055
##########
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxBroadcastBuildSideCache.scala:
##########
@@ -86,18 +86,42 @@ object VeloxBroadcastBuildSideCache
def getOrBuildBroadcastHashTable(
broadcast: Broadcast[BuildSideRelation],
broadcastContext: BroadcastHashJoinContext): BroadcastHashTable = {
+ // #region debug-point bhj-hash-memory-size-cache-check
+ val cached =
buildSideRelationCache.getIfPresent(broadcastContext.buildHashTableId)
+ logWarning(
+ s"[debug][bhj-hash-memory-size] getOrBuildBroadcastHashTable " +
+ s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
+ s"cacheHit=${cached != null}, " +
+
s"metricDefined=${broadcastContext.hashTableMemorySizeMetric.isDefined}")
+ // #endregion debug-point bhj-hash-memory-size-cache-check
buildSideRelationCache
.get(
broadcastContext.buildHashTableId,
(_: String) => {
+ // #region debug-point bhj-hash-memory-size-loader-enter
+ logWarning(
+ s"[debug][bhj-hash-memory-size] cache loader entered " +
+ s"for buildHashTableId=${broadcastContext.buildHashTableId}")
+ // #endregion debug-point bhj-hash-memory-size-loader-enter
val (pointer, relation, droppedDuplicates) = broadcast.value match {
case columnar: ColumnarBuildSideRelation =>
columnar.buildHashTable(broadcastContext)
case unsafe: UnsafeColumnarBuildSideRelation =>
unsafe.buildHashTable(broadcastContext)
}
+ // #region debug-point bhj-hash-memory-size-value
+ val hashTableMemoryUsage =
HashJoinBuilder.getHashTableMemoryUsage(pointer)
+ logWarning(
+ s"[debug][bhj-hash-memory-size] built hash table " +
+ s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
+ s"pointer=$pointer, " +
+ s"hashTableMemoryUsage=$hashTableMemoryUsage, " +
+
s"metricDefined=${broadcastContext.hashTableMemorySizeMetric.isDefined}")
+ broadcastContext.hashTableMemorySizeMetric.foreach(_ +=
hashTableMemoryUsage)
+ // #endregion debug-point bhj-hash-memory-size-value
Review Comment:
hash table memory usage is fetched via a native (JNI) call even when the
metric is not defined; this adds overhead on the critical path. Only call into
JNI when the metric is present.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxBroadcastBuildSideCache.scala:
##########
@@ -86,18 +86,42 @@ object VeloxBroadcastBuildSideCache
def getOrBuildBroadcastHashTable(
broadcast: Broadcast[BuildSideRelation],
broadcastContext: BroadcastHashJoinContext): BroadcastHashTable = {
+ // #region debug-point bhj-hash-memory-size-cache-check
+ val cached =
buildSideRelationCache.getIfPresent(broadcastContext.buildHashTableId)
+ logWarning(
+ s"[debug][bhj-hash-memory-size] getOrBuildBroadcastHashTable " +
+ s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
+ s"cacheHit=${cached != null}, " +
+
s"metricDefined=${broadcastContext.hashTableMemorySizeMetric.isDefined}")
+ // #endregion debug-point bhj-hash-memory-size-cache-check
buildSideRelationCache
.get(
broadcastContext.buildHashTableId,
(_: String) => {
+ // #region debug-point bhj-hash-memory-size-loader-enter
+ logWarning(
+ s"[debug][bhj-hash-memory-size] cache loader entered " +
+ s"for buildHashTableId=${broadcastContext.buildHashTableId}")
+ // #endregion debug-point bhj-hash-memory-size-loader-enter
Review Comment:
This 'debug-point' logging uses logWarning and will execute for every cache
load, adding noise. Prefer logDebug behind log.isDebugEnabled, or remove.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxBroadcastBuildSideCache.scala:
##########
@@ -196,7 +220,12 @@ object VeloxBroadcastBuildSideCache
buildSideRelationCache.get(
broadcastHashTableId,
(_: String) => {
- logInfo(s"Deserializing hash table on executor for broadcast ID:
$broadcastHashTableId")
+ // #region debug-point bhj-hash-memory-size-deserialize-enter
+ logWarning(
+ s"[debug][bhj-hash-memory-size] deserializeOnExecutor entered " +
+ s"broadcastId=$broadcastHashTableId, " +
+
s"deserializeMetricDefined=${deserializeHashTableTimeMetric.isDefined}")
+ // #endregion debug-point bhj-hash-memory-size-deserialize-enter
Review Comment:
This debug log was changed from logInfo to logWarning and adds extra
debug-only fields. Consider restoring the original logInfo (or making it
logDebug) to avoid warning-level noise during normal deserialization.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashJoinExecTransformer.scala:
##########
@@ -181,8 +181,23 @@ case class BroadcastHashJoinExecTransformer(
metrics.get("buildHashTableTime"),
metrics.get("serializeHashTableTime"),
metrics.get("deserializeHashTableTime"),
- metrics.get("serializedHashTableSize")
+ metrics.get("serializedHashTableSize"),
+ metrics.get("hashTableMemorySize")
)
+ // #region debug-point bhj-hash-memory-size-driver-attach
+ val cacheCodeSource = Option(
+ org.apache.gluten.execution.VeloxBroadcastBuildSideCache
+ .getClass
+ .getProtectionDomain
+ .getCodeSource
+ ).map(_.getLocation).map(_.toString).getOrElse("unknown")
+ logWarning(
+ s"[debug][bhj-hash-memory-size] BHJ driver context " +
+ s"buildHashTableId=$buildBroadcastTableId, " +
+
s"hashTableMemorySizeMetricDefined=${context.hashTableMemorySizeMetric.isDefined},
" +
+
s"buildHashTableTimeMetricDefined=${context.buildHashTableTimeMetric.isDefined},
" +
+ s"cacheCodeSource=$cacheCodeSource")
+ // #endregion debug-point bhj-hash-memory-size-driver-attach
Review Comment:
This adds warning-level debug logging and also uses reflection
(ProtectionDomain/CodeSource), which can be relatively expensive and is not
needed for the metric feature. Prefer removing, or at least guard with
log.isDebugEnabled and logDebug without the CodeSource lookup.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxBroadcastBuildSideCache.scala:
##########
@@ -86,18 +86,42 @@ object VeloxBroadcastBuildSideCache
def getOrBuildBroadcastHashTable(
broadcast: Broadcast[BuildSideRelation],
broadcastContext: BroadcastHashJoinContext): BroadcastHashTable = {
+ // #region debug-point bhj-hash-memory-size-cache-check
+ val cached =
buildSideRelationCache.getIfPresent(broadcastContext.buildHashTableId)
+ logWarning(
+ s"[debug][bhj-hash-memory-size] getOrBuildBroadcastHashTable " +
+ s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
+ s"cacheHit=${cached != null}, " +
+
s"metricDefined=${broadcastContext.hashTableMemorySizeMetric.isDefined}")
+ // #endregion debug-point bhj-hash-memory-size-cache-check
Review Comment:
These log lines are tagged as debug but use logWarning, which can spam
executor logs in production. Consider downgrading to logDebug (guarded by
log.isDebugEnabled) or removing the block entirely.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxBroadcastBuildSideRDD.scala:
##########
@@ -29,19 +30,28 @@ case class VeloxBroadcastBuildSideRDD(
broadcasted: broadcast.Broadcast[BuildSideRelation],
broadcastContext: BroadcastHashJoinContext,
isBNL: Boolean = false)
- extends BroadcastBuildSideRDD(sc, broadcasted) {
+ extends BroadcastBuildSideRDD(sc, broadcasted)
+ with Logging {
override def genBroadcastBuildSideIterator(): Iterator[ColumnarBatch] = {
- val offload = broadcasted.value.asReadOnlyCopy() match {
+ val relationCopy = broadcasted.value.asReadOnlyCopy()
+ val offload = relationCopy match {
case columnar: ColumnarBuildSideRelation =>
columnar.offload
case unsafe: UnsafeColumnarBuildSideRelation =>
unsafe.isOffload
}
+ // #region debug-point bhj-hash-memory-size-branch
+ logWarning(
+ s"[debug][bhj-hash-memory-size] VeloxBroadcastBuildSideRDD " +
+ s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
+ s"relationClass=${relationCopy.getClass.getName}, " +
+ s"isBNL=$isBNL, " +
+ s"offload=$offload")
+ // #endregion debug-point bhj-hash-memory-size-branch
Review Comment:
This block logs debug-only information at warning level. That can be very
noisy because this method runs per task/partition. Please downgrade to logDebug
(guarded by log.isDebugEnabled) or remove.
##########
backends-velox/src/main/scala/org/apache/gluten/execution/VeloxSerializedBroadcastRDD.scala:
##########
@@ -39,6 +41,12 @@ case class VeloxSerializedBroadcastRDD(
s"VeloxSerializedBroadcastRDD expects
SerializedHashTableBroadcastRelation, " +
s"but got: ${other.getClass.getName}")
}
+ // #region debug-point bhj-hash-memory-size-serialized-deserialize
+ logWarning(
+ s"[debug][bhj-hash-memory-size] VeloxSerializedBroadcastRDD " +
+ s"buildHashTableId=${broadcastContext.buildHashTableId}, " +
+
s"deserializeMetricDefined=${broadcastContext.deserializeHashTableTimeMetric.isDefined}")
+ // #endregion debug-point bhj-hash-memory-size-serialized-deserialize
Review Comment:
This is debug-only logging but uses logWarning, which can spam executor
logs. Please downgrade to logDebug (guarded) or remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
