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

[Michael McMahon]

David Schlosnagle wrote:
> 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:
>
>   
Thanks for confirming that. I'll switch it back to the original
chained .append().

- Michael.