RFR (2nd round) 8071627: Code refactoring to override == operator of Symbol*

[Stefan Karlsson]

On 2015-04-16 01:25, Calvin Cheung wrote:
> Hi Stefan,
>
> Thanks for your review.
>
> On 4/15/2015 2:50 PM, Stefan Karlsson wrote:
>> Hi Calvin,
>>
>> On 2015-04-15 21:56, Calvin Cheung wrote:
>>> Please review this second version of the fix.
>>>
>>> This version has 2 new functions (equals() and not_equals()) in the 
>>> Symbol class.
>>> It replaces the Symbol* == and != comparisons with those 2 function 
>>> calls.
>>>
>>> Pro:
>>>
>>> It has a much smaller changeset than the first version.
>>>
>>> Con:
>>>
>>> Someone may by mistake introduce a new line of (Symbol* == Symbol*).
>>>
>>> We will mitigate this by enhancing our internal static analysis tool 
>>> to flag the
>>> above code in the future.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8071627
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/
>>
>> This is a much less intrusive change than the previous patch. Thanks.
>>
>>
>> http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/classfile/classFileParser.cpp.patch
>>
>> Is there a reason why you added not_equals:
>> -              if (name != vmSymbols::object_initializer_name()) {
>> +              if (name->not_equals(vmSymbols::object_initializer_name())) {
>>
>> instead of just:
>> +              if (!name->equals(vmSymbols::object_initializer_name())) {
> We think that it's clearer to have not_equals() than using the ! as in 
> the above.

OK. IMHO, negating boolean expressions with ! is a common construct so 
I'm not sure not_equals is clearer.

>>
>> http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/classfile/systemDictionary.cpp.udiff.html
>> !     assert(parsed_name->not_equals(NULL), "Sanity");
>>
>>
>> You use symbol == NULL but not symbol != NULL, which seems 
>> inconsistent to me.
> I'm not sure I understand this comment.
> Are you referring to the following section of the udiff?
> *** 1104,1115 ****
> --- 1104,1115 ----
>       Exceptions::_throw_msg(THREAD_AND_LOCATION,
>         vmSymbols::java_lang_SecurityException(), message);
>     }
>
>     if (!HAS_PENDING_EXCEPTION) {
> !     assert(parsed_name != NULL, "Sanity");
> !     assert(class_name == NULL || class_name == parsed_name, "name 
> mismatch");
> !     assert(parsed_name->not_equals(NULL), "Sanity");
> !     assert(class_name == NULL || class_name->equals(parsed_name), 
> "name mismatch");
>       // Verification prevents us from creating names with dots in 
> them, this
>       // asserts that that's the case.
>       assert(is_internal_format(parsed_name),
>              "external class name format used internally");
>
> I don't see anything incorrect there.

I'm not talking about being correct or incorrect. I would have expected 
to see either:

1) Use pointer (in)equality when checking for NULL:
class_name == NULL
parsed_name != NULL

or

2) strictly use the (not_)equals function when checking for NULL:
class_name->equals(NULL)
parsed_name->not_equals(NULL)
or even:
class_name->equals(NULL)
!parsed_name->equals(NULL))

but not what you have today, where you mix the two ways to NULL check
class_name == NULL
parsed_name->not_equals(NULL)


>> http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/oops/symbol.hpp.udiff.html
>>
>> *+   inline bool equals(const Symbol* other) const {*
>> *+     if (this && other) {
>> *
>> First, pointers should be checked null checked with == or !=. See:
>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#StyleGuide-Miscellaneous
>
> I will fix it.
>
>>
>> Second, I recall some discussion that null checking the 'this' 
>> pointer is undefined behavior. Though, we do it in other places so 
>> this isn't worse, I think.
>>
>> Did you use your previous patch to find all Symbol* compares?
>
> Yes. Essentially comment out the bodies of the == and != operators in 
> SymbolRef.
> Rebuilding hotspot resulting a lot of "undefined reference to 
> SymbolRef::operator==" link errors.
> Then go through those error and change a == b to a->equals(b) and a != 
> b to a->not_equals(b).
> Obviously change SymbolRef back to Symbol*.

Great.

One more thing:
http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/oops/symbol.hpp.udiff.html

*+       if (this->_identity_hash != other->_identity_hash) {*
*+         return false;*
*+       }*


Is this correct? The current code sets up _identity_hash with 
os::random, so it seems like to Symbols with equal contents would most 
likely going to get different _identity_hash codes. Or will this be 
changed later?

Thanks,
StefanK
>
> thanks,
> Calvin
>
>>
>> Thanks,
>> StefanK
>>
>>>
>>> Tests:
>>>     JPRT (almost done)
>>>     Will do more perf testing after JPRT
>>>
>>> thanks,
>>> Calvin
>>
>