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

[Peter Levart]

On 11/29/2016 10:58 PM, Claes Redestad wrote:
> Hi Peter,
>
> On 11/29/2016 01:28 PM, Peter Levart wrote:
>> 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.
>
> Yes, I didn't suggest otherwise. :-)
>
>>
>>>
>>>>
>>>> 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/
>
> I think pushing this as 8035424 is OK.
>
> I know I just told you to remove it, but wouldn't the slightly awkward 
> for loops read better as:
>
> char c = current();
> while (!(...)) {
>   c = getNext();
> }

They would if c = getNext(); was equivalent to { advance(); c = 
current(); }, but it was not. It was defined as: { c = current(); 
advance(); }.

I could re-introduce a different getNext() with the desired behavior, 
but here's another variant with a more "streaming" approach which tries 
to re-use the logic for parsing the identifier:

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


What do you think of this one?

Regards, Peter


>
> Thanks!
>
> /Claes