Improve performance of WinProcess#getEnvironmentVariables (#69)

pom.xml

@@ -21,6 +21,7 @@
     <gitHubRepo>jenkinsci/${project.artifactId}</gitHubRepo>
     <!-- TODO fix violations -->
     <spotbugs.threshold>High</spotbugs.threshold>
+    <jmh.version>1.34</jmh.version>
   </properties>
 
   <scm>
@@ -63,6 +64,12 @@
         </exclusion>
       </exclusions>
     </dependency>
+    <dependency>
+      <groupId>org.hamcrest</groupId>
+      <artifactId>hamcrest-core</artifactId>
+      <version>2.2</version>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>org.junit.jupiter</groupId>
       <artifactId>junit-jupiter</artifactId>
@@ -73,6 +80,18 @@
       <artifactId>junit-vintage-engine</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.openjdk.jmh</groupId>
+      <artifactId>jmh-core</artifactId>
+      <version>${jmh.version}</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.openjdk.jmh</groupId>
+      <artifactId>jmh-generator-annprocess</artifactId>
+      <version>${jmh.version}</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
   <licenses>

src/main/java/org/jvnet/winp/WinProcess.java

@@ -1,8 +1,9 @@
 package org.jvnet.winp;
 
 import edu.umd.cs.findbugs.annotations.CheckReturnValue;
-import java.util.TreeMap;
 import java.util.Iterator;
+import java.util.StringTokenizer;
+import java.util.TreeMap;
 import java.util.logging.Logger;
 
 import static java.util.logging.Level.FINE;
@@ -134,8 +135,9 @@ public synchronized String getCommandLine() {
      *      The process may be dead or there is not enough security privileges.
      */
     public synchronized TreeMap<String,String> getEnvironmentVariables() {
-        if(envVars==null)
+        if(envVars==null) {
             parseCmdLineAndEnvVars();
+        }
         return envVars;
     }
 
@@ -151,27 +153,20 @@ private void parseCmdLineAndEnvVars() {
         String s = Native.getCmdLineAndEnvVars(pid);
         if(s==null)
             throw new WinpException("Failed to obtain for PID="+pid);
-        int sep = s.indexOf('\0');
-        commandline = s.substring(0,sep);
+        // a double null indicates the end of the envvars (but the string can be longer!) so truncate here.
+        int end = s.indexOf("\0\0");
+        if (end != -1) {
+            s = s.substring(0, end);
+        }
+        StringTokenizer st = new StringTokenizer(s, "\0", false);
+        commandline = st.nextToken();
         envVars = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
-        s = s.substring(sep+1);
-
-        while(s.length()>0) {
-            sep = s.indexOf('\0');
-            if(sep==0)  return;
-
-            String t;
-            if(sep==-1) {
-                t = s;
-                s = "";
-            } else {
-                t = s.substring(0,sep);
-                s = s.substring(sep+1);
+        while (st.hasMoreElements()) {
+            final String kv = st.nextToken();
+            int sep = kv.indexOf('=');
+            if  (sep!=-1) { // be defensive. not exactly sure when this happens, but see HUDSON-4034
+                envVars.put(kv.substring(0, sep), kv.substring(sep + 1));
             }
-
-            sep = t.indexOf('=');
-            if  (sep!=-1) // be defensive. not exactly sure when this happens, but see HUDSON-4034
-                envVars.put(t.substring(0,sep),t.substring(sep+1));
         }
     }
 

src/test/java/BenchmarkRunner.java

@@ -0,0 +1,6 @@
+public class BenchmarkRunner {
+
+    public static void main(String[] args) throws Exception {
+        org.openjdk.jmh.Main.main(args);
+    }
+}

src/test/java/org/jvnet/winp/PlatformSpecificProcessTest.java

@@ -26,6 +26,8 @@
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.anEmptyMap;
+import static org.hamcrest.Matchers.hasEntry;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThrows;
@@ -34,6 +36,10 @@
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
@@ -52,7 +58,17 @@
  */
 @RunWith(Parameterized.class)
 public class PlatformSpecificProcessTest extends ProcessSpawningTest {
-
+
+    private static final Set<String> ARCHITECTURE_DEPENDANT_ENVIRONMENT_VARS;
+
+    static {
+        ARCHITECTURE_DEPENDANT_ENVIRONMENT_VARS = new HashSet<>();
+        ARCHITECTURE_DEPENDANT_ENVIRONMENT_VARS.add("PROCESSOR_ARCHITEW6432");
+        ARCHITECTURE_DEPENDANT_ENVIRONMENT_VARS.add("PROCESSOR_ARCHITECTURE");
+        ARCHITECTURE_DEPENDANT_ENVIRONMENT_VARS.add("PROGRAMFILES");
+        ARCHITECTURE_DEPENDANT_ENVIRONMENT_VARS.add("COMMONPROGRAMFILES");
+    }
+
     private final ExecutablePlatform executablePlatform; 
 
     public PlatformSpecificProcessTest(ExecutablePlatform p) {
@@ -109,7 +125,42 @@ public void getCommandLine_shouldNotFailIfTheProcessIsDead() throws Exception {
                 e.getWin32ErrorCode(),
                 equalTo(UserErrorType.PROCESS_IS_NOT_RUNNING.getSystemErrorCode()));
     }
-
+
+    @Test
+    public void getEnvironmentVariables_shouldReturnCorrectValues() throws Exception {
+        Process p = spawnTestApp();
+        WinProcess wp = new WinProcess(p);
+        // spawned processes should inherit our environment variables
+        Map<String, String> inheritedEnv = System.getenv();
+        Map<String, String> processEnv = wp.getEnvironmentVariables();
+
+        // environment variable that start with = is just some funky stuff!
+        // and there are some that start with "=" that won't show up in set e.g. =ExitCode =CLINK.SCRIPTS
+        for (Map.Entry<String, String> entry : inheritedEnv.entrySet()) {
+            if (!(entry.getKey().equals("") || entry.getKey().startsWith("=") || // :-o  special
+                    ARCHITECTURE_DEPENDANT_ENVIRONMENT_VARS.contains(entry.getKey())))  {
+                assertThat(processEnv, hasEntry(entry.getKey(), entry.getValue()));
+            }
+        }
+        // the extra env added by spawnTestApp
+        assertThat(processEnv, hasEntry("TEST", "foobar"));
+
+        // what remains?
+        Map<String, String> remaining = new HashMap<>();
+        for (Map.Entry<String, String>  kv : processEnv.entrySet()) {
+            if (! (inheritedEnv.containsKey(kv.getKey()) || kv.getKey().equals("TEST") || kv.getKey().equals("")
+                    || kv.getKey().startsWith("="))) {
+                // some vars are changed by windows depending on if you are a 32bit process running in a 64 bit os or 64 on 64.
+                // just filter those out
+                if (!ARCHITECTURE_DEPENDANT_ENVIRONMENT_VARS.contains(kv.getKey())) {
+                    remaining.put(kv.getKey(), kv.getValue());
+                }
+            }
+        }
+        assertThat(remaining, anEmptyMap());
+    }
+
+
     @Test
     public void getEnvironmentVariables_shouldFailIfTheProcessIsDead() throws Exception {
         Process p = spawnTestApp();

src/test/java/org/jvnet/winp/ProcessEnvironmentVariableBenchmark.java

@@ -0,0 +1,32 @@
+package org.jvnet.winp;
+
+import org.openjdk.jmh.annotations.*;
+import org.openjdk.jmh.infra.Blackhole;
+
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.concurrent.TimeUnit;
+
+public class ProcessEnvironmentVariableBenchmark {
+
+    @Benchmark
+    @OutputTimeUnit(TimeUnit.MILLISECONDS)
+    @BenchmarkMode(Mode.AverageTime)
+    @Fork(1)
+    public void testEnvRetreival(ProcessState state, Blackhole blackhole) {
+        WinProcess wp = new WinProcess(state.p);
+        Map<String, String> env = wp.getEnvironmentVariables();
+        //System.out.println(env.get("USERNAME"));
+        blackhole.consume(env);
+    }
+
+    @Benchmark
+    @OutputTimeUnit(TimeUnit.MILLISECONDS)
+    @BenchmarkMode(Mode.AverageTime)
+    @Fork(1)
+    public void testNativeRaw(ProcessState state, Blackhole blackhole) {
+        WinProcess wp = new WinProcess(state.p);
+        String str = Native.getCmdLineAndEnvVars(wp.getPid());
+        blackhole.consume(str);
+    }
+}

src/test/java/org/jvnet/winp/ProcessState.java

@@ -0,0 +1,33 @@
+package org.jvnet.winp;
+
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.State;
+
+import java.io.File;
+import java.io.IOException;
+
+@State(Scope.Benchmark)
+public class ProcessState {
+
+    private static final File NULL_FILE = new File("NUL:");
+
+    public Process p;
+
+    @Setup(Level.Trial)
+    public void setUp() throws IOException {
+        ProcessBuilder pb = new ProcessBuilder();
+        pb.command("ping", "-t", "localhost");
+        pb.redirectError(NULL_FILE);
+        pb.redirectOutput(NULL_FILE);
+        p = pb.start();
+    }
+
+    @TearDown(Level.Trial)
+    public void tearDown() throws InterruptedException {
+        p.destroyForcibly();
+        p.waitFor();
+    }
+}