RFR: 8035424: (reflect) Performance problem in sun.reflect.generics.parser.SignatureParser

[Peter Levart]

Hi Claes,

On 11/29/2016 06:34 PM, Claes Redestad wrote:
> Hi Peter,
>
> On 11/29/2016 08:57 AM, Peter Levart wrote:
>> Hi,
>>
>> What about not using StringBuilder at all?
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/8170467_SignatureParser/webrev.01/ 
>>
>
> your patch idea is rather clever but might prove to be better across 
> the board with no
> need for any magic numbers, which is nice.

It also improves the parseIdentifier() method - no double copying of 
chars as done when using StringBuilder.

>
>>
>> There's also some mockery in getNext()/current()/advance() that uses 
>> exceptions for control flow which can be fixed while changing this 
>> part of code. The asserts in getNext()/current()/advance() could even 
>> fail if getNext()/advance() was called after already returning EOI 
>> (== character ':')...
>>
>
> (mockery :D)
>
> Unfortunately I've already pushed Max's patch, but there's also 
> https://bugs.openjdk.java.net/browse/JDK-8035424
> which asks for getting rid of the exceptional flow using a suggestion 
> similar to yours, and I don't mind sponsoring
> another go at this. I suggest we move ahead with your patch using that 
> bug ID.
>
> (getNext - and matches - appear unused and could be removed rather 
> than fixed?)

Right, do you want just removal of exceptions or the whole thing?

I reduced copying further. The 'input' field is a char[], but could 
simply be a String. No need to extract input String's chars into an 
array 1st:

http://cr.openjdk.java.net/~plevart/jdk9-dev/8035424_SignatureParser.performance/webrev.01/


Regards, Peter

>
> /Claes
>
>> Regards, Peter
>>
>>
>> On 11/29/2016 03:03 PM, Claes Redestad wrote:
>>> Hi Andrej,
>>>
>>> On 2016-11-29 14:39, Andrej Golovnin wrote:
>>>> Hi Claes,
>>>>
>>>> 76     public static final int CLASS_NAME_SB_SIZE = 48;
>>>>
>>>> Why is this constant public?
>>>
>>> Good catch, made it private.
>>>
>>>> Btw. for our product this value is too small. We have packages with
>>>> longer names. I would use 64. :-)
>>>
>>> There's no one size fits all: something around 48 is likely to
>>> help most common cases, while not noticeably penalizing
>>> shorter names. I'd consider it a bad move to regress apps
>>> with short-to-normal names to get a small gain on apps with
>>> very long names
>>>
>>> Thanks!
>>>
>>> /Claes
>>>
>>>>
>>>> Best regards,
>>>> Andrej Golovnin
>>>>
>>>> On Tue, Nov 29, 2016 at 2:18 PM, Claes Redestad
>>>> <claes.redestad at oracle.com> wrote:
>>>>> Hi,
>>>>>
>>>>> please review this patch provided by Max Kanat-Alexander[1] to 
>>>>> improve
>>>>> performance of sun.reflect.generics.parser.SignatureParser by 
>>>>> reducing
>>>>> number of StringBuilders created and the rate at which they are 
>>>>> resized
>>>>> during typical usage.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~redestad/8170467/webrev.01/
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170467
>>>>>
>>>>> Thanks!
>>>>>
>>>>> /Claes
>>>>>
>>>>> [1]
>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-November/045046.html 
>>>>>
>>>
>>
>