As a micro-optimization, inline the "Callstack.enabled" guard. The java-land function call overhead of the morally no-op Callstack#push/pop was profiled to be ~1.4% CPU in a benchmark of loading a BUILD file that was particularly heavy in Skylark function calls.

Alternatives considered: writing code that I hoped would be more amenable to the JIT choosing to inline the function call. I couldn't get this to work.

RELNOTES: None
PiperOrigin-RevId: 188350132

3 files changed, 19 insertions, 15 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index b5ceb0c039..e445e46cf8 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -440,10 +440,14 @@ public abstract class BaseFunction implements SkylarkValue {
     canonicalizeArguments(arguments, loc);
 
     try {
-      Callstack.push(this);
+      if (Callstack.enabled) {
+        Callstack.push(this);
+      }
       return call(arguments, ast, env);
     } finally {
-      Callstack.pop();
+      if (Callstack.enabled) {
+        Callstack.pop();
+      }
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Callstack.java b/src/main/java/com/google/devtools/build/lib/syntax/Callstack.java
index 30217e91c2..352acef0eb 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Callstack.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Callstack.java
@@ -27,7 +27,9 @@ import java.util.List;
  * instrumentation. It should not be used by normal Skylark interpreter logic.
  */
 public class Callstack {
-  private static boolean enabled;
+  // This field should be read, but not written, directly in order to avoid the overhead of a
+  // method call.
+  static boolean enabled;
   private static final ThreadLocal<List<Object>> callstack =
       ThreadLocal.withInitial(ArrayList::new);
 
@@ -36,22 +38,16 @@ public class Callstack {
   }
 
   public static void push(ASTNode node) {
-    if (enabled) {
-      callstack.get().add(node);
-    }
+    callstack.get().add(node);
   }
 
   public static void push(BaseFunction function) {
-    if (enabled) {
-      callstack.get().add(function);
-    }
+    callstack.get().add(function);
   }
 
   public static void pop() {
-    if (enabled) {
-      List<Object> threadStack = callstack.get();
-      threadStack.remove(threadStack.size() - 1);
-    }
+    List<Object> threadStack = callstack.get();
+    threadStack.remove(threadStack.size() - 1);
   }
 
   public static List<Object> get() {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
index db5d4e658e..182c6567a4 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
@@ -62,14 +62,18 @@ public abstract class Expression extends ASTNode {
    */
   public final Object eval(Environment env) throws EvalException, InterruptedException {
     try {
-      Callstack.push(this);
+      if (Callstack.enabled) {
+        Callstack.push(this);
+      }
       try {
         return doEval(env);
       } catch (EvalException ex) {
         throw maybeTransformException(ex);
       }
     } finally {
-      Callstack.pop();
+      if (Callstack.enabled) {
+        Callstack.pop();
+      }
     }
   }