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

[Claes Redestad]

On 11/29/2016 02:30 PM, Peter Levart wrote:
>
>
>
> 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?

Fair point. I think this is easier to follow and should do the trick 
just as nicely.

What you have here is fine, but in this day and age of compact strings I 
wonder if return mar.replace('/', '.') might have both a readability and 
a slight performance edge (assuming signatures are almost always ASCII).

Thanks!

/Claes

>
> Regards, Peter
>
>
>>
>> Thanks!
>>
>> /Claes
>