RFR: 8220366: Optimize Symbol handling in ClassVerifier and SignatureStream

[Lois Foltan]

On 3/14/2019 12:15 PM, Claes Redestad wrote:
> On 2019-03-14 16:47, Lois Foltan wrote:
>>
>> Hi Claes,
>>
>> Looks like a good improvement! 
>
> Thanks!
>
>> A couple of comments:
>>
>> - src/hotspot/share/oops/symbol.cpp - new method 
>> Symbol::make_permanent()
>>    I have a concern that the while (true) loop could be infinite, can 
>> you have Coleen ok this part of the change since she is the expert in 
>> regards to Symbol refcounts?
>
> I've checked this with Coleen. The race handling logic here mimics that
> of increment_ and decrement_refcount. A live lock condition where two or
> more threads keep trying to update a refcount indefinitely should be
> extremely rare, in practice they should always resolve within a few
> iterations.
>
>>
>> - src/hotspot/share/runtime/signature.cpp
>>    The removal of SignatureVerifier::invalid_name_char(char c) method 
>> seems fine except that the new switch at line #496 does not include 
>> the ';' case, why?
>
> Note that the ';' case is handled above the switch. Admittedly, this
> expression can be simplified a bit:
>
> diff -r 89b091544fd0 src/hotspot/share/runtime/signature.cpp
> --- a/src/hotspot/share/runtime/signature.cpp    Thu Mar 14 16:50:03 
> 2019 +0100
> +++ b/src/hotspot/share/runtime/signature.cpp    Thu Mar 14 17:09:35 
> 2019 +0100
> @@ -479,10 +479,9 @@
>      case 'L':
>        for (index = index + 1; index < limit; ++index) {
>          char c = type[index];
> -        if (c == ';') {
> -          return index + 1;
> -        }
>          switch (c) {
> +          case ';':
> +            return index + 1;
>            case '\0': case '.': case '[':
>              return -1;
>            default: ; // fall through
>
> Maybe that's clearer? Seeing that the code is ASSERT-only since
> JDK-8219579 the changes in this area are mostly cosmetic.

Thanks for the explanation, I missed the earlier check for ';' in the 
if.  I like having it moved into the switch statement.  And I see Coleen 
reviewed at the same time I did, so I think this change looks good.  I 
don't need to see another webrev.
Lois

>
>> - copyrights need to be updated in various files
>
> Sure.
>
> /Claes