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
