Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

[David Schlosnagle]

On Sun, Apr 17, 2011 at 1:27 PM, Ulf Zibis <Ulf.Zibis at gmx.de> wrote:
> Am 16.04.2011 16:45, schrieb David Schlosnagle:
>>
>> One minor nit in ProcessEnvironment.java
>>
>>  336     private static void addToEnv(StringBuilder sb, String name,
>> String val) {
>>  337         sb.append(name+"="+val+'\u0000');
>>  338     }
>>
>> I think it should be as follows to avoid implicitly constructing
>> another StringBuilder and the extra copying overhead;
>>
>>  336     private static void addToEnv(StringBuilder sb, String name,
>> String val) {
>>  337         sb.append(name).append('=').append(val).append('\u0000');
>>  338     }
>>
>
> Because this suggestion was from me, I must admit, that I didn't prove, if
> javac actually uses the existing StringBuilder sb, or forces another
> StringBuilder to instantiate. I just assumed, javac would use the existing
> one. So to be done:
> - examine the byte code
> - if not yet optimized: then new RFE for javac

Ulf,

I'm not the right person to comment on whether javac could safely
optimize away the StringBuilder creation here per JLS 15.18.1 [1] [2],
but in this situation I'd assume that the expression
name+"="+val+'\u0000' must return a "result is a reference to a String
object (newly created, unless the expression is a compile-time
constant expression (§15.28))that is the concatenation of the two
operand strings" in which case I don't think javac can optimize it
away.

Some IDEs such as IntelliJ offer code inspections to find and fix such
items, see "String concatenation inside 'StringBuffer.append()'" [3]:
    This inspection reports any instances of String concatenation used as the
    argument to StringBuffer.append(), StringBuilder.append() or
    Appendable.append(). Such calls may profitably be turned into chained
    append calls on the existing StringBuffer/Builder/Appendable, saving the
    cost of an extra StringBuffer/Builder allocation.

    This inspection ignores compile time evaluated String concatenations, which
    when converted to chained append calls would only worsen performance.

Here's a quick `javap -c -private` dump of these two different methods
in a test class, showing that the changes I'm recommending simplify
the bytecode and avoid the extra StringBuilder construction and
copying of underlying characters:


Before:
    private static void addToEnv(StringBuilder sb, String name, String val) {
        sb.append(name+"="+val+'\u0000');
    }

Yields:

    private static void addToEnv(java.lang.StringBuilder,
java.lang.String, java.lang.String);
      Code:
       0:	aload_0
       1:	new	#2; //class java/lang/StringBuilder
       4:	dup
       5:	invokespecial	#3; //Method java/lang/StringBuilder."<init>":()V
       8:	aload_1
       9:	invokevirtual	#8; //Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
       12:	ldc	#9; //String =
       14:	invokevirtual	#8; //Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
       17:	aload_2
       18:	invokevirtual	#8; //Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
       21:	iconst_0
       22:	invokevirtual	#10; //Method
java/lang/StringBuilder.append:(C)Ljava/lang/StringBuilder;
       25:	invokevirtual	#11; //Method
java/lang/StringBuilder.toString:()Ljava/lang/String;
       28:	invokevirtual	#8; //Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
       31:	pop
       32:	return

----

After:
    private static void addToEnv(StringBuilder sb, String name, String val) {
        sb.append(name).append('=').append(val).append('\u0000');
    }

Yields:

    private static void addToEnv(java.lang.StringBuilder,
java.lang.String, java.lang.String);
      Code:
       0:	aload_0
       1:	aload_1
       2:	invokevirtual	#8; //Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
       5:	bipush	61
       7:	invokevirtual	#10; //Method
java/lang/StringBuilder.append:(C)Ljava/lang/StringBuilder;
       10:	aload_2
       11:	invokevirtual	#8; //Method
java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
       14:	iconst_0
       15:	invokevirtual	#10; //Method
java/lang/StringBuilder.append:(C)Ljava/lang/StringBuilder;
       18:	pop
       19:	return

[1] http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.18.1
[2] http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.18.1.2
[3] http://www.jetbrains.com/idea/documentation/inspections/StringConcatenationInsideStringBufferAppend.html


- Dave