Fix performance of WinProcess.getEnvironmentVariables() #69

jtnord · 2022-01-31T14:48:06Z

The performance of the code was no longer efficient in a Java5 world where String.substring involves memory copying to prevent holding onto larger strings preventing GC.

Benchmark showed the following:

# Run complete. Total time: 00:05:01

REMEMBER: The numbers below are just data. To gain reusable insights, you need to follow up on
why the numbers are the way they are. Use profilers (see -prof, -lprof), design factorial
experiments, perform baseline and negative tests that provide experimental control, make sure
the benchmarking environment is safe on JVM/OS/HW level, ask for reviews from the domain experts.
Do not assume the numbers tell you what you want them to tell.

Benchmark                                          Mode  Cnt  Score   Error  Units
ProcessEnvironmentVariableBenchmark.testNativeRaw  avgt    5  0.122 ± 0.024  ms/op
ProcessEnvironmentVariableBenchmark.testNew        avgt    5  0.135 ± 0.021  ms/op
ProcessEnvironmentVariableBenchmark.testOriginal   avgt    5  1.069 ± 0.112  ms/op

the java code used to take approximately .9ms per call and now takes approx 0.01ms

test code was only checking that the returned environment variable map ontained the "TEST" environment variable, however if the code returned incorrectly then the test would still pass. This adds a unit test to ensure future changes do not regress behaviour

jtnord · 2022-01-31T14:53:35Z

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

@@ -159,36 +157,23 @@ 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);
-        envVars = new TreeMap<String,String>(CASE_INSENSITIVE_COMPARATOR);


the old CASE_INSENSITIVE_COMPARATOR was inefficient - running String.toUpperCase repeatedly on insertion.

jtnord · 2022-01-31T14:55:16Z

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

-            } else {
-                t = s.substring(0,sep);
-                s = s.substring(sep+1);


the other part of the inefficiency was just chopping the string down, and creating substrings from it. Far more efficient to scan the entire String once and chop it.

jtnord · 2022-01-31T14:55:52Z

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

+        if (end != -1) {
+            s = s.substring(0, end);
+        }
+        StringTokenizer st = new StringTokenizer(s, "\0", false);


despite the fact that javadoc points you to use String.split(string regexp) it is no where near as performant according to the benchmark.

jtnord · 2022-01-31T14:56:58Z

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

+        }
+        StringTokenizer st = new StringTokenizer(s, "\0", false);
+        commandline = st.nextToken();
+        envVars = new TreeMap<String,String>(String.CASE_INSENSITIVE_ORDER);


using String.CASE_INSENSITIVE_ORDER which can shortcut the comparison as it uses String.compareIgnoreCase() which can in the optimim just look at the first character, so avoids an entire upper-casing of each string on comparison.

jtnord · 2022-01-31T14:58:21Z

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

+    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");
+    }


there may be more - these where what I observed on my windows 11 x64 environment.

jtnord · 2022-01-31T15:02:54Z

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

+        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


I could not find any documentation on this (starting with =, but they exist and are funky!
so just excluded them from the test

e.g.

set echo %=ExitCode%

oleg-nenashev

LGTM, bumping to Java 1.8 is perfectly fine at this moment.
Note that I do not longer have release environment for Windows components and Maven Central. Please contact @batmat and @MarkEWaite to discuss who will take care of thios component, and I will help with getting permissions. Bonus points for a Jenkins devlist thread

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

This is jenkinsci/winp#69

jtnord · 2022-01-31T17:25:10Z

Note that I do not longer have release environment for Windows components and Maven Central.

Are you the only maintainer currently @oleg-nenashev? As the module appears to be used outside of Jenkins (still in my Idea distrubution) is the Jenkins dev-list the best place for this to happen, as the Jenkins needs may well be different from others.

jtnord · 2022-02-11T21:49:48Z

Note that I do not longer have release environment for Windows components and Maven Central. Please contact @batmat and @MarkEWaite to discuss who will take care of thios component, and I will help with getting permissions. Bonus points for a Jenkins devlist thread

https://groups.google.com/g/jenkinsci-dev/c/JmdP7Z0_ppc

Also cc @segrey

basil

Thanks for the PR!

jtnord · 2022-09-27T08:53:19Z

Thanks for merging Basil, I had forgotten about this one.

jtnord added 3 commits January 31, 2022 14:01

Add a benchmark and test a new implementation

70ffce1

Switch implementation and remove the old code

c756737

jtnord force-pushed the perf branch from 2f7f74d to c756737 Compare January 31, 2022 14:50

remove dead code

9fbad94

jtnord commented Jan 31, 2022

View reviewed changes

oleg-nenashev self-requested a review January 31, 2022 15:13

oleg-nenashev approved these changes Jan 31, 2022

View reviewed changes

src/main/java/org/jvnet/winp/WinProcess.java Outdated Show resolved Hide resolved

jtnord added a commit to jtnord/jenkins that referenced this pull request Jan 31, 2022

pickup a version of winp with the fix.

ba14d5d

This is jenkinsci/winp#69

jtnord mentioned this pull request Jan 31, 2022

[JENKINS-67681] ProcessTree$Windows#killAll is slow on Windows jenkinsci/jenkins#6236

Merged

10 tasks

revert import .*

8575c79

jtnord mentioned this pull request May 23, 2022

transfer repo to Jenkinsci org #70

Closed

basil added 2 commits September 26, 2022 11:25

Merge remote-tracking branch 'origin/master' into perf

84c7a73

Extract JMH version into property for easy updating

9098eb3

basil approved these changes Sep 26, 2022

View reviewed changes

basil merged commit 2c33aff into jenkinsci:master Sep 26, 2022

basil added the enhancement label Sep 26, 2022

basil mentioned this pull request May 6, 2024

Support for windows-arm64 #112

Closed

basil mentioned this pull request Aug 7, 2024

Revert "Use official Jenkins CI to build and test winp" #128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix performance of WinProcess.getEnvironmentVariables() #69

Fix performance of WinProcess.getEnvironmentVariables() #69

jtnord commented Jan 31, 2022 •

edited

Loading

jtnord Jan 31, 2022

jtnord Jan 31, 2022

jtnord Jan 31, 2022

jtnord Jan 31, 2022

jtnord Jan 31, 2022

jtnord Jan 31, 2022

oleg-nenashev left a comment

jtnord commented Jan 31, 2022

jtnord commented Feb 11, 2022

basil left a comment

jtnord commented Sep 27, 2022

Fix performance of WinProcess.getEnvironmentVariables() #69

Fix performance of WinProcess.getEnvironmentVariables() #69

Conversation

jtnord commented Jan 31, 2022 • edited Loading

jtnord Jan 31, 2022

Choose a reason for hiding this comment

jtnord Jan 31, 2022

Choose a reason for hiding this comment

jtnord Jan 31, 2022

Choose a reason for hiding this comment

jtnord Jan 31, 2022

Choose a reason for hiding this comment

jtnord Jan 31, 2022

Choose a reason for hiding this comment

jtnord Jan 31, 2022

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

jtnord commented Jan 31, 2022

jtnord commented Feb 11, 2022

basil left a comment

Choose a reason for hiding this comment

jtnord commented Sep 27, 2022

jtnord commented Jan 31, 2022 •

edited

Loading