Copilot commented on code in PR #12323:
URL: https://github.com/apache/gluten/pull/12323#discussion_r3450986515
##########
gluten-arrow/src/main/scala/org/apache/gluten/runtime/Runtime.scala:
##########
@@ -81,7 +81,8 @@ object Runtime {
s"Runtime instance already released: $handle, ${resourceName()},
${priority()}")
}
RuntimeJniWrapper.releaseRuntime(handle)
-
+ ntm.release()
+ nmm.release()
Review Comment:
Releasing the native runtime handle before releasing `ntm`/`nmm` can be
unsafe if the runtime depends on these managers during teardown. Also, if
`releaseRuntime(handle)` throws, `ntm.release()`/`nmm.release()` won’t run,
potentially leaking native resources. Consider releasing `ntm`/`nmm` before
`releaseRuntime(handle)` and using a `try`/`finally` (or equivalent) so
sub-resources are released even if one step fails.
##########
gluten-arrow/src/main/scala/org/apache/gluten/memory/NativeMemoryManager.scala:
##########
@@ -81,14 +80,14 @@ object NativeMemoryManager {
override def release(): Unit = {
if (!released.compareAndSet(false, true)) {
throw new GlutenException(
- s"Memory manager instance already released: $handle,
${resourceName()}, ${priority()}")
+ s"Memory manager instance already released: $handle")
}
def dump(): String = {
SparkMemoryUtil.prettyPrintStats(
- s"[${resourceName()}]",
+ s"[nmm]",
new KnownNameAndStats() {
- override def name: String = resourceName()
+ override def name: String = "nmm"
override def stats: MemoryUsageStats = collectUsage()
})
Review Comment:
The `Impl` constructor still takes `name: String`, but the dump label and
stat `name` are now hard-coded to `nmm`. This loses caller-provided identity
(and can make logs ambiguous if multiple memory managers exist). Prefer using
the passed `name` (e.g., `s\"[$name]\"` and `override def name: String = name`)
so the behavior matches the API.
##########
gluten-arrow/src/main/scala/org/apache/gluten/memory/NativeMemoryManager.scala:
##########
@@ -38,12 +37,12 @@ trait NativeMemoryManager {
def addSpiller(spiller: Spiller): Unit
def hold(): Unit
def getHandle(): Long
+ def release(): Unit
}
Review Comment:
Adding an abstract `release(): Unit` to a (likely public) trait is a
binary/source breaking change for any external implementations. If this trait
is part of a public API surface, consider mitigating by (a) providing a default
implementation (if feasible), (b) introducing a new sub-trait that adds
`release()`, or (c) making the trait `sealed`/package-private if it’s intended
to be internal-only.
--
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]
