RFR [8054221] StringJoiner imlementation optimization

[Martin Buchholz]

I agree that assertions like assertThrows belongs in your test framework.
 For this change my preference would be to revert to the previous code
without expectedExceptions but author preference wins.


On Wed, Aug 6, 2014 at 9:16 AM, roger riggs <roger.riggs at oracle.com> wrote:

> Hi,
>
> If a function like assertThrows was included in TestNG, it would make
> sense to use it.
> The TestNG expectedException mechanism is adequate and creating a one-off
> extension to TestNG
> would raise the maintenance costs without adding much value.
>
> Roger
>
>
>
> On 8/6/2014 10:13 AM, Martin Buchholz wrote:
>
>> I have more comments, but this is good to go.
>>
>>    69     @Test(expectedExceptions = {NullPointerException.class})
>>
>>
>> Controversial - many people, including myself, think this is too magical
>> without adding much value.  My own preference is assertThrows e.g. in
>> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/
>> test/tck/JSR166TestCase.java?revision=1.121&view=markup
>>
>> Microbenchmarks undermeasure the impact of allocation, which increases
>> with
>> heap size.  Real world java applications struggle with allocation/gc, not
>> cpu.  So don't measure allocation against cpu directly in microbenchmarks.
>>
>>
>>
>>
>>
>> On Wed, Aug 6, 2014 at 2:47 AM, Ivan Gerasimov <ivan.gerasimov at oracle.com
>> >
>> wrote:
>>
>>  On 06.08.2014 10:14, Martin Buchholz wrote:
>>>
>>>     39     private final static String PREFIXES[] = {"", "{", "@#$%"};
>>>
>>> This C style syntax is not good Java style - use String[] instead.
>>>
>>>    Thanks! fixed
>>>
>>>
>>>     177         if (addLen == 0) { 178             compactElts(); 179
>>>           return size == 0 ? "" : elts[0]; 180         }
>>>
>>> I'm concerned about the extra String[8] created by compactElts.  We
>>> assume
>>> that StringJoiners are all temporary objects, so don't bother creating
>>> that
>>> shorter String[8] to hold the one sole element on compaction - just reuse
>>> the original array (but should we null out the entries? probably)
>>>
>>>
>>> The benchmark showed that allocating a new array is faster than
>>> nullifying
>>> the existing one.
>>> But if we set the entries to null in the loop that is already there, it
>>> will probably be no slower than allocation.
>>>
>>>
>>> Here's the updated webrev:
>>> http://cr.openjdk.java.net/~igerasim/8054221/2/webrev/
>>>
>>> Sincerely yours,
>>> Ivan
>>>
>>>
>>>
>>> On Tue, Aug 5, 2014 at 12:19 PM, Ivan Gerasimov <
>>> ivan.gerasimov at oracle.com
>>>
>>>> wrote:
>>>>   But if we do that, I think we should optimize size == 0 as well, thus:
>>>>
>>>>> if (addLen == 0 && size <= 1)
>>>>>>   return (size == 0) ? "" : elts[0];
>>>>>>
>>>>>>  Yes, done.
>>>>>
>>>>>   Or we can just call compactElts() if addLenn == 0, so it will work
>>>>> for
>>>>>
>>>> any size.
>>>>
>>>> Updated the webrev at the same location:
>>>> http://cr.openjdk.java.net/~igerasim/8054221/1/webrev/
>>>>
>>>> Sincerely yours,
>>>> Ivan
>>>>
>>>>
>>>>
>>>
>