(spark) branch master updated: [SPARK-60340][CORE] Avoid System.getenv on every JavaUtils.isTesting call

[gu...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

HyukjinKwon pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/spark.git


The following commit(s) were added to refs/heads/master by this push:
     new 5e2cef87a190 [SPARK-60340][CORE] Avoid System.getenv on every JavaUtils.isTesting call
5e2cef87a190 is described below

commit 5e2cef87a19035f5dc3b840aaab3c78b46399ce8
Author: Sebastian Hillig <se...@databricks.com>
AuthorDate: Tue Oct 20 21:15:41 2026 +0900

    [SPARK-60340][CORE] Avoid System.getenv on every JavaUtils.isTesting call
    
    ### What changes were proposed in this pull request?
    This PR makes `JavaUtils.isTesting()` look up the `SPARK_TESTING` environment variable once, caching it in a `private static final boolean`, instead of calling `System.getenv("SPARK_TESTING")` on every invocation. The `spark.testing` system property is still read on every call, and the evaluation order (env first, then property) is unchanged.
    
    `Utils.isTesting` and `SparkEnvUtils.isTesting` delegate to this method, so they benefit too.
    
    ### Why are the changes needed?
    On Linux, `System.getenv(String)` goes through `java.lang.ProcessEnvironment`, which wraps the name in a new byte[]-backed `Variable` on every lookup, so each `isTesting()` call allocates (56 bytes per call in the measurement below). `isTesting` is on hot paths. For example, `AnalysisHelper.assertNotAnalysisRule` calls it from the `LogicalPlan` overrides of `transformDownWithPruning` / `transformUpWithPruning` / `transformAllExpressionsWithPruning`, which run once per plan node visite [...]
    
    A JVM's environment cannot change after startup, so caching the env lookup is safe. The system property cannot be cached: tests set and clear it at runtime (`SparkTestSuite.beforeAll`, `BenchmarkBase`, `KryoSerializerBenchmark`). The only code that mutates the process environment reflectively (`SparkConnectClientSuite.withEnvs`, `JavaXmlSuite.setEnv`) touches `SPARK_REMOTE` / `SPARK_LOCAL_IP`, not `SPARK_TESTING`. Code that sets `SPARK_TESTING` for a child process via `ProcessBuilder. [...]
    
    ### Does this PR introduce _any_ user-facing change?
    No.
    
    ### How was this patch tested?
    Existing tests:
    
    ```
    build/sbt common-utils-java/test common-utils/test 'network-common/testOnly *JavaUtilsSuite' catalyst/compile
    ```
    
    All passed (common-utils-java: 25, common-utils: 121, JavaUtilsSuite: 2), and catalyst compiled.
    
    I also ran a manual allocation check (not committed): 1M calls on one thread, measured with `ThreadMXBean.getThreadAllocatedBytes`, `SPARK_TESTING` unset, JDK 17, after warm-up:
    
    | | bytes / 1M calls | time / 1M calls |
    |---|---|---|
    | before (`System.getenv` + `System.getProperty`) | 56,000,000 (56 B/call) | ~25-43 ms |
    | after | 0 | ~2-2.5 ms |
    
    The same check confirmed that setting and clearing `spark.testing` at runtime still flips the result (`false` / `true` / `false`), and that with `SPARK_TESTING=1` the result is `true` as before.
    
    ### Was this patch authored or co-authored using generative AI tooling?
    Generated-by: Isaac (Claude Opus 5.5)
    
    This pull request and its description were written by Isaac.
    
    Closes #59593 from sebastianhillig-db/istesting-cache-getenv.
    
    Authored-by: Sebastian Hillig <se...@databricks.com>
    Signed-off-by: Hyukjin Kwon <hy...@databricks.com>
---
 .../apache/spark/network/util/JavaUtilsSuite.java  | 28 ++++++++++++++++++++++
 .../org/apache/spark/network/util/JavaUtils.java   |  7 +++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/common/network-common/src/test/java/org/apache/spark/network/util/JavaUtilsSuite.java b/common/network-common/src/test/java/org/apache/spark/network/util/JavaUtilsSuite.java
index 64edc0edae6f..a690f3148960 100644
--- a/common/network-common/src/test/java/org/apache/spark/network/util/JavaUtilsSuite.java
+++ b/common/network-common/src/test/java/org/apache/spark/network/util/JavaUtilsSuite.java
@@ -18,12 +18,15 @@ package org.apache.spark.network.util;
 
 import java.io.File;
 import java.io.IOException;
+import java.lang.reflect.Field;
 import java.nio.file.Files;
+import java.util.Map;
 
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.*;
 import static org.junit.jupiter.api.Assumptions.assumeFalse;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
 public class JavaUtilsSuite {
 
@@ -90,4 +93,29 @@ public class JavaUtilsSuite {
     assertEquals(1, JavaUtils.listFiles(symlink).size());
     assertEquals(1, JavaUtils.listPaths(symlink).size());
   }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  public void testIsTestingReadsSparkTestingEnvOnce() throws Exception {
+    // sbt and Maven run tests with SPARK_TESTING set.
+    assumeTrue(System.getenv("SPARK_TESTING") != null);
+    assertTrue(JavaUtils.isTesting());
+
+    Map<String, String> env = System.getenv();
+    Field field = env.getClass().getDeclaredField("m");
+    field.setAccessible(true);
+    Map<String, String> writableEnv = (Map<String, String>) field.get(env);
+    String envValue = writableEnv.remove("SPARK_TESTING");
+    String propValue = System.clearProperty("spark.testing");
+    try {
+      assertNull(System.getenv("SPARK_TESTING"));
+      // The env lookup is cached at class initialization, so it is unaffected.
+      assertTrue(JavaUtils.isTesting());
+    } finally {
+      writableEnv.put("SPARK_TESTING", envValue);
+      if (propValue != null) {
+        System.setProperty("spark.testing", propValue);
+      }
+    }
+  }
 }
diff --git a/common/utils-java/src/main/java/org/apache/spark/network/util/JavaUtils.java b/common/utils-java/src/main/java/org/apache/spark/network/util/JavaUtils.java
index 2cf4570488ee..225aeab694fc 100644
--- a/common/utils-java/src/main/java/org/apache/spark/network/util/JavaUtils.java
+++ b/common/utils-java/src/main/java/org/apache/spark/network/util/JavaUtils.java
@@ -696,11 +696,16 @@ public class JavaUtils {
     return new String(in.readAllBytes(), StandardCharsets.UTF_8);
   }
 
+  // The process environment is immutable, and System.getenv(String) allocates on every lookup,
+  // which shows up on hot paths such as Catalyst's per-transform isTesting check. The
+  // spark.testing system property is still read on each call since tests set and clear it.
+  private static final boolean SPARK_TESTING_ENV_SET = System.getenv("SPARK_TESTING") != null;
+
   /**
    * Indicates whether Spark is currently running unit tests.
    */
   public static boolean isTesting() {
-    return System.getenv("SPARK_TESTING") != null || System.getProperty("spark.testing") != null;
+    return SPARK_TESTING_ENV_SET || System.getProperty("spark.testing") != null;
   }
 
   /**


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org