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

[Ulf Zibis]

Hi David,

much thanks for your detailed effort.

-Ulf

Am 17.04.2011 22:50, schrieb 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
>