LuciferYang opened a new pull request, #12792: URL: https://github.com/apache/gluten/pull/12792
### What changes were proposed in this pull request? `MemoryTargetUtil.toUniqueName` kept a `ConcurrentHashMap` from name to counter and never evicted from it, so the map grew for as long as the JVM lived. That would be bounded if names came from a fixed set, but they do not: `NativePlanEvaluator.create` derives its runtime name from a JVM-global `AtomicInteger`, and that name reaches `toUniqueName` through `Runtimes.contextInstance`, `NativeMemoryManager`, `ReservationListeners`, and `TreeMemoryConsumer.Node`'s constructor. Each such name appears exactly once, so its entry is written, never read again, and never removed. Both sides accumulate. On an executor it is one entry per whole-stage transformer per task, from `VeloxIteratorApi`. On the driver it is one per native validation, since `VeloxValidatorApi#doNativeValidateWithFailureReason` and `#doNativeValidateExpression` each build an evaluator inside `TaskResources.runUnsafe` while planning; a long-lived driver such as a Thrift server validates candidate operators for every query, so it accumulates faster per unit time than an executor and releases nothing between queries. This uses a single `AtomicLong` for all names. The suffix only has to make the name unique, because `TreeMemoryConsumer#newChild` keys siblings by name and throws on a collision, and a shared sequence does that with no per-name state. The `long` also replaces an `Integer` that would wrap after 2^31 targets on the fixed names such as `Gluten.Tree`. Suffixes are no longer dense per name. The names that change most are the high-cardinality ones that leaked: each appeared once, so its suffix was always `.0`, and `NativePlanEvaluator-7.0` now reads something like `NativePlanEvaluator-7.913`. The fixed names were already climbing JVM-globally, `Gluten.Tree` once per task, so only their scale changes. Nothing depends on the number: it is a label for `SparkMemoryUtil`'s stats map and pretty printer, it is not parsed anywhere in the repo, `memory.proto` carries no name field, and it never crosses JNI, where only `backendName` is passed. A bounded or weak cache would be worse than either: evicting a live target's entry lets the counter restart and hand a second sibling the same name, turning `newChild`'s collision check from unreachable code into a task failure. ### How was this patch tested? New `MemoryTargetUtilTest` with four tests. The load-bearing one is `testNamingHoldsNoPerNameState`: it reflects over the class's declared fields and asserts none is a `Map` or `Collection`, then asserts three calls consume exactly three numbers from the shared sequence. Both halves are needed. Asserting only on the suffixes would pass for an implementation that keeps an entry per name and takes the number from a shared counter, which is to say it would pass while still leaking. A `@VisibleForTesting sequenceForTesting()` supports the second half, following the same pattern already used for static state in `DynamicOffHeapSizingMemoryTarget`. The other three tests cover suffixes advancing across different names, repeated names staying distinct (the property `newChild` depends on), and 8 threads x 500 names all distinct. Each assertion was checked against a mutant, restoring the source after every run: | mutant | caught by | | --- | --- | | keep the per-name map, take the suffix from the shared sequence (still leaks, no visible symptom) | `Naming should hold no per-name state, found java.util.Map LOOKUP` | | the original per-name counter | the same assertion, plus `Suffix did not advance across names: 0, 0` | `mvn -Pspark-3.5 -pl gluten-core test` gives 55 tests passing. Cross-version `test-compile` passes on spark-3.3, spark-3.4, spark-4.0 with scala-2.13, and spark-4.1 with scala-2.13. One scope note: `RegularMemoryConsumer` calls `toUniqueName` with an externally supplied name, which is the pattern that made the growth unbounded. It has no production construction site today, and after this change an external name is no longer a hazard anyway, since there is no per-name state left to accumulate. Removing the dead class is a separate concern. ### Was this patch authored or co-authored using generative AI tooling? No Closes #12790 -- 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]
