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

Peter Levart peter.levart at gmail.com
Tue Nov 29 21:28:23 UTC 2016

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/8170467SignatureParser/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 CLASSNAMESBSIZE = 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

More information about the core-libs-dev mailing list