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