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: [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]
