You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "bhollis-dbx (via GitHub)" <gi...@apache.org> on 2026/09/16 16:13:40 UTC
[PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
bhollis-dbx opened a new pull request, #58865:
URL: https://github.com/apache/spark/pull/58865
### What changes were proposed in this pull request?
Replace five per-`TreeNode` `BestEffortLazyVal` instances with nullable volatile cache fields and lock-free lazy accessors. The affected properties are `treePatternBits`, `containsChild`, `height`, `hashCode`, and `allChildren`.
Add a regression test for allocation when constructing unused `Literal` expressions.
### Why are the changes needed?
Every tree node currently allocates five memoizers and their initializer closures even when their properties are never read. Catalyst creates large numbers of expression and plan nodes, making this unused cache state expensive.
On the measured JVM, constructing an unused `Literal` falls from 280 to 81 allocated bytes, saving 199 bytes (71%) per node. The new accessors preserve lock-free concurrent initialization, single-result publication, origin behavior, and retry after computation failure.
This is kind of a messy optimization, but it may be worth it given the memory savings across all TreeNodes. It's a judgement call whether that win is worth the harder-to-read code.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added an unused-`Literal` construction allocation test and ran:
`build/sbt 'catalyst/testOnly *TreeNodeSuite'`
All 37 tests passed.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Codex (GPT-5)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #58865: [SPARK-59580][SQL] Reduce TreeNode cache allocation
URL: https://github.com/apache/spark/pull/58865
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on PR #58865:
URL: https://github.com/apache/spark/pull/58865#issuecomment-5828503389
cc @cloud-fan @LuciferYang can you PTAL at this PR?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "bhollis-dbx (via GitHub)" <gi...@apache.org>.
bhollis-dbx commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4125645574
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -99,6 +100,30 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val bean = ManagementFactory.getThreadMXBean.asInstanceOf[com.sun.management.ThreadMXBean]
Review Comment:
Done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4101727380
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -99,6 +100,30 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val bean = ManagementFactory.getThreadMXBean.asInstanceOf[com.sun.management.ThreadMXBean]
+ assert(bean.isThreadAllocatedMemorySupported)
+ bean.setThreadAllocatedMemoryEnabled(true)
Review Comment:
**Nit (P3):** setThreadAllocatedMemoryEnabled(true) flips a JVM-wide setting that is never restored, so it persists into every later test in the same JVM (and on an early failure before any teardown). The practical impact is small because HotSpot enables this measurement by default, but it would be cleaner to capture the prior isThreadAllocatedMemoryEnabled() value and restore it in a finally block -- or rely on the default and not toggle it -- so the test leaves no global JVM state behind.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -99,6 +100,30 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val bean = ManagementFactory.getThreadMXBean.asInstanceOf[com.sun.management.ThreadMXBean]
+ assert(bean.isThreadAllocatedMemorySupported)
+ bean.setThreadAllocatedMemoryEnabled(true)
+ val count = 100000
+ val literals = new Array[Literal](count)
+
+ var i = 0
+ while (i < count) {
+ Literal(1)
+ i += 1
+ }
+
+ val before = bean.getCurrentThreadAllocatedBytes
+ i = 0
+ while (i < count) {
+ literals(i) = Literal(1)
+ i += 1
+ }
+ val bytesPerLiteral = (bean.getCurrentThreadAllocatedBytes - before) / count
+
+ assert(bytesPerLiteral < 220, s"unused Literal allocated $bytesPerLiteral bytes")
Review Comment:
**Non-blocking (P2):** This assertion encodes an absolute per-Literal allocation total, but that total is dominated by the irreducible cost of constructing a Literal and by JIT escape-analysis / object-layout differences across JVMs, not by the eager cache state the test targets. The PR measured 81 bytes on the author's JVM, while the apache/spark catalyst CI job (Zulu JDK 17) measured 344 and this test fails there ("344 was not less than 220"), so CI is currently red and the check will misfire on other JVMs even when the optimization is correct. Consider isolating the delta attributable to the removed cache wrappers -- e.g. compare a freshly constructed untouched node against a tolerance derived from that wrapper overhead, or measure only the allocation attributable to the cache fields -- rather than asserting an absolute per-Literal size; at minimum, raise the bound well above the observed cross-JVM floor.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "bhollis-dbx (via GitHub)" <gi...@apache.org>.
bhollis-dbx commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4139664935
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -98,7 +104,50 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
+case class ConcurrentTreeNode(barrier: CyclicBarrier, computeCount: AtomicInteger)
+ extends LeafNode {
+ override def output: Seq[Attribute] = Nil
+
+ override protected def getDefaultTreePatternBits: BitSet = {
+ computeCount.incrementAndGet()
+ barrier.await(10, TimeUnit.SECONDS)
+ new BitSet(TreePattern.maxId)
+ }
+}
+
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val literal = Literal(1)
+ val cacheFields = classOf[TreeNode[_]].getDeclaredFields
+ .filter(_.getName.endsWith("CacheValue"))
+
+ assert(cacheFields.length === 5)
+ cacheFields.foreach { field =>
+ field.setAccessible(true)
+ assert(field.get(literal) === null, s"${field.getName} was eagerly initialized")
+ }
+ }
+
+ test("concurrent cache initialization returns and reuses the selected value") {
+ val barrier = new CyclicBarrier(2)
+ val computeCount = new AtomicInteger
+ val node = ConcurrentTreeNode(barrier, computeCount)
+ val executor = ThreadUtils.newDaemonFixedThreadPool(2, "tree-node-cache-test")
+ val executionContext = ExecutionContext.fromExecutorService(executor)
+ try {
+ val first = Future(node.treePatternBits)(executionContext)
Review Comment:
Tested the rest of the branches
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "bhollis-dbx (via GitHub)" <gi...@apache.org>.
bhollis-dbx commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4125641424
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -99,6 +100,30 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val bean = ManagementFactory.getThreadMXBean.asInstanceOf[com.sun.management.ThreadMXBean]
+ assert(bean.isThreadAllocatedMemorySupported)
+ bean.setThreadAllocatedMemoryEnabled(true)
Review Comment:
Done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4103813357
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -99,6 +100,30 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val bean = ManagementFactory.getThreadMXBean.asInstanceOf[com.sun.management.ThreadMXBean]
Review Comment:
The capability probe uses `assert` where it should use `assume`: when `isThreadAllocatedMemorySupported` is false the test fails instead of being skipped, and the sibling `CodeCompilerSuite` already uses `assume` for exactly this kind of JVM-capability gate. The `.asInstanceOf[com.sun.management.ThreadMXBean]` on line 104 also runs before any check, so a JVM that doesn't provide that implementation throws `ClassCastException` first.
Take `val mxBean = ManagementFactory.getThreadMXBean`, `assume(mxBean.isInstanceOf[com.sun.management.ThreadMXBean])`, then cast to `bean` and `assume(bean.isThreadAllocatedMemorySupported)`. Non-HotSpot JVMs then skip rather than error, and HotSpot behavior is unchanged.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -118,8 +136,19 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
* A BitSet of tree patterns for this TreeNode and its subtree. If this TreeNode and its
* subtree contains a pattern `P`, the corresponding bit for `P.id` is set in this BitSet.
*/
- private val _treePatternBits = new BestEffortLazyVal[BitSet](() => getDefaultTreePatternBits)
- override def treePatternBits: BitSet = _treePatternBits()
+ @volatile private var treePatternBitsCacheValue: BitSet = _
+ override def treePatternBits: BitSet = {
+ var value = treePatternBitsCacheValue
+ if (value eq null) {
+ val computed = getDefaultTreePatternBits
Review Comment:
These caches use null as the "not computed yet" sentinel, which implicitly requires every compute to be non-null. `BestEffortLazyVal` guarded that with `assert newValue != null` plus a documented contract; the hand-rolled CAS drops it. If a compute ever returns null, `compareAndSet(this, null, null)` succeeds harmlessly, the field stays null, and the value is recomputed on every call, a silent cache miss.
All five current computes are non-null, so this isn't reachable today; `getDefaultTreePatternBits` is the only one a subclass can override. A one-line comment noting compute must be non-null is enough, no need to add the assert back.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "bhollis-dbx (via GitHub)" <gi...@apache.org>.
bhollis-dbx commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4125622910
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -118,8 +136,19 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
* A BitSet of tree patterns for this TreeNode and its subtree. If this TreeNode and its
* subtree contains a pattern `P`, the corresponding bit for `P.id` is set in this BitSet.
*/
- private val _treePatternBits = new BestEffortLazyVal[BitSet](() => getDefaultTreePatternBits)
- override def treePatternBits: BitSet = _treePatternBits()
+ @volatile private var treePatternBitsCacheValue: BitSet = _
+ override def treePatternBits: BitSet = {
+ var value = treePatternBitsCacheValue
+ if (value eq null) {
+ val computed = getDefaultTreePatternBits
Review Comment:
Documented that cache computations must not return null because null is the uninitialized sentinel.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4129480283
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -98,7 +104,50 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
+case class ConcurrentTreeNode(barrier: CyclicBarrier, computeCount: AtomicInteger)
+ extends LeafNode {
+ override def output: Seq[Attribute] = Nil
+
+ override protected def getDefaultTreePatternBits: BitSet = {
+ computeCount.incrementAndGet()
+ barrier.await(10, TimeUnit.SECONDS)
+ new BitSet(TreePattern.maxId)
+ }
+}
+
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val literal = Literal(1)
+ val cacheFields = classOf[TreeNode[_]].getDeclaredFields
+ .filter(_.getName.endsWith("CacheValue"))
+
+ assert(cacheFields.length === 5)
+ cacheFields.foreach { field =>
+ field.setAccessible(true)
+ assert(field.get(literal) === null, s"${field.getName} was eagerly initialized")
+ }
+ }
+
+ test("concurrent cache initialization returns and reuses the selected value") {
+ val barrier = new CyclicBarrier(2)
+ val computeCount = new AtomicInteger
+ val node = ConcurrentTreeNode(barrier, computeCount)
+ val executor = ThreadUtils.newDaemonFixedThreadPool(2, "tree-node-cache-test")
+ val executionContext = ExecutionContext.fromExecutorService(executor)
+ try {
+ val first = Future(node.treePatternBits)(executionContext)
Review Comment:
**Non-blocking (P2):** The post-construction test only calls `treePatternBits`, even though this patch hand-wires five separate field, VarHandle, and accessor paths. For example, swapping the same-typed `heightCache` and `hashCodeCache` handles, or dropping publication for a collection cache, still leaves both new tests passing while height/hash results can be crossed or collections rebuilt on every call.
**Verification:**
- **Regression:** Exercise every cache accessor on a controlled tree, verify the expected value and corresponding field population, repeat the access, and assert that the cached object is retained; the checks must fail if the integer VarHandles are swapped or any collection accessor omits publication.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4105578480
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -99,6 +100,30 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val bean = ManagementFactory.getThreadMXBean.asInstanceOf[com.sun.management.ThreadMXBean]
+ assert(bean.isThreadAllocatedMemorySupported)
+ bean.setThreadAllocatedMemoryEnabled(true)
+ val count = 100000
+ val literals = new Array[Literal](count)
+
+ var i = 0
+ while (i < count) {
+ Literal(1)
+ i += 1
+ }
+
+ val before = bean.getCurrentThreadAllocatedBytes
+ i = 0
+ while (i < count) {
+ literals(i) = Literal(1)
+ i += 1
Review Comment:
**Non-blocking (P2):** `literals` is never observed after this loop. Because the array and its `Literal` graph do not escape, an optimizing JVM can eliminate these stores and allocations, letting the test pass even if eager cache allocation regresses. Please consume the populated array through an observable, non-inlinable sink after measuring so the allocation graph remains live.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -118,8 +136,19 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
* A BitSet of tree patterns for this TreeNode and its subtree. If this TreeNode and its
* subtree contains a pattern `P`, the corresponding bit for `P.id` is set in this BitSet.
*/
- private val _treePatternBits = new BestEffortLazyVal[BitSet](() => getDefaultTreePatternBits)
- override def treePatternBits: BitSet = _treePatternBits()
+ @volatile private var treePatternBitsCacheValue: BitSet = _
+ override def treePatternBits: BitSet = {
+ var value = treePatternBitsCacheValue
+ if (value eq null) {
+ val computed = getDefaultTreePatternBits
Review Comment:
**Non-blocking (P2):** This publication path is now owned by `TreeNode`, so `BestEffortLazyValSuite` can no longer exercise it. Please add deterministic TreeNode-level concurrent-initialization coverage that verifies racing calls return the same CAS-selected object and a later call reuses it; one representative accessor is sufficient for the shared pattern.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "bhollis-dbx (via GitHub)" <gi...@apache.org>.
bhollis-dbx commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4125647951
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -99,6 +100,30 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val bean = ManagementFactory.getThreadMXBean.asInstanceOf[com.sun.management.ThreadMXBean]
+ assert(bean.isThreadAllocatedMemorySupported)
+ bean.setThreadAllocatedMemoryEnabled(true)
+ val count = 100000
+ val literals = new Array[Literal](count)
+
+ var i = 0
+ while (i < count) {
+ Literal(1)
+ i += 1
+ }
+
+ val before = bean.getCurrentThreadAllocatedBytes
+ i = 0
+ while (i < count) {
+ literals(i) = Literal(1)
+ i += 1
Review Comment:
Done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #58865:
URL: https://github.com/apache/spark/pull/58865#issuecomment-6225381128
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/6f6d1912ad60d5093ff12cfb15f915d2bfe75f90
- merged into branch-4.x https://github.com/apache/spark/commit/034e2f6059180b1960b230396caa2ab4c05c2ab1
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #58865:
URL: https://github.com/apache/spark/pull/58865#issuecomment-5834193345
Thanks for the ping. I reviewed this revision and left the current findings in the review.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"conversation:5828503389","thread_id":"conversation:5828503389","verdict_sha256":"da05fd0c31513076494e57b29f72f89fcbd79dd088a774823a151437cdc65861"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "bhollis-dbx (via GitHub)" <gi...@apache.org>.
bhollis-dbx commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4125639479
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala:
##########
@@ -99,6 +100,30 @@ case class FakeLeafPlan(child: LogicalPlan)
case class FakeCurryingProduct(x: Expression)(val y: Int)
class TreeNodeSuite extends SparkFunSuite with SQLHelper {
+ test("constructing unused Literals does not allocate eager cache state") {
+ val bean = ManagementFactory.getThreadMXBean.asInstanceOf[com.sun.management.ThreadMXBean]
+ assert(bean.isThreadAllocatedMemorySupported)
+ bean.setThreadAllocatedMemoryEnabled(true)
+ val count = 100000
+ val literals = new Array[Literal](count)
+
+ var i = 0
+ while (i < count) {
+ Literal(1)
+ i += 1
+ }
+
+ val before = bean.getCurrentThreadAllocatedBytes
+ i = 0
+ while (i < count) {
+ literals(i) = Literal(1)
+ i += 1
+ }
+ val bytesPerLiteral = (bean.getCurrentThreadAllocatedBytes - before) / count
+
+ assert(bytesPerLiteral < 220, s"unused Literal allocated $bytesPerLiteral bytes")
Review Comment:
Replaced the allocation measurement with a direct assertion that all five cache fields remain uninitialized after constructing a node. This removes the JVM-specific threshold, MXBean dependency, global-state mutation, and dead-code risk.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59580][SQL] Reduce TreeNode cache allocation [spark]
Posted by "bhollis-dbx (via GitHub)" <gi...@apache.org>.
bhollis-dbx commented on code in PR #58865:
URL: https://github.com/apache/spark/pull/58865#discussion_r4125650781
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala:
##########
@@ -118,8 +136,19 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]]
* A BitSet of tree patterns for this TreeNode and its subtree. If this TreeNode and its
* subtree contains a pattern `P`, the corresponding bit for `P.id` is set in this BitSet.
*/
- private val _treePatternBits = new BestEffortLazyVal[BitSet](() => getDefaultTreePatternBits)
- override def treePatternBits: BitSet = _treePatternBits()
+ @volatile private var treePatternBitsCacheValue: BitSet = _
+ override def treePatternBits: BitSet = {
+ var value = treePatternBitsCacheValue
+ if (value eq null) {
+ val computed = getDefaultTreePatternBits
Review Comment:
Added a barrier-based test verifying racing callers return the CAS-selected instance and later calls reuse it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org