RFR (2nd round) 8071627: Code refactoring to override == operator of Symbol*
Stefan Karlsson
stefan.karlsson at oracle.com
Fri Apr 17 06:40:44 UTC 2015
On 2015-04-17 01:23, Calvin Cheung wrote:
> On 4/15/2015 11:13 PM, Ioi Lam wrote:
>> On 4/15/15 11:02 PM, Stefan Karlsson wrote:
>>> 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)
>>>
>>
>> I think it's better to use (symbol == NULL) everywhere. Calling
>> symbol->equals(NULL) while symbol itself is NULL is confusing -- it
>> raises questions like "will this crash in debug mode", so we'd better
>> avoid it.
> I've fixed all those NULL comparison back to (symbol == NULL) or
> (symbol != NULL).
>
>>
>> In our static analysis tool, we need to flags checks for (symbol1 ==
>> symbol2), but should allow (symbol == NULL).
>>
>> - Ioi
>>
>>>
>>>>> 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?
> I removed that comparison for now.
>
> Updated webrev:
> http://cr.openjdk.java.net/~ccheung/8071627/webrev.02/
>
> Files modified since last time:
> src/share/vm/classfile/classFileParser.cpp
> src/share/vm/classfile/systemDictionary.cpp
> src/share/vm/oops/symbol.hpp
> src/share/vm/prims/methodHandles.cpp
It's be easier for the reviewers if you also provide webrev with the
delta of your patches.
You've now taken away the identity hash code check, which I suggested
was a bug. I think that might affect the performance. Before the removal
the functions almost always exited after the identity hash code and
didn't have to do the string compare, but now I think it does the string
compare much more often. I'm afraid you will have to redo the
performance measurements now.
And just a style comment:
*+ inline bool equals(const Symbol* other) const {*
*+ if ((this != NULL) && (other != NULL)) {*
*+ int len = this->utf8_length();*
*+ if (len != other->utf8_length()) {*
*+ return false;*
*+ }*
*+ return (strncmp((const char*)(this->base()), (const char*)(other->base()), len) == 0);*
*+ } else {*
*+ return (this == other);*
*+ }*
*+ }*
*+ inline bool not_equals(const Symbol* other) const {*
*+ return !(this->equals(other));*
*+ }
*
There's a large amount of unnecessary parentheses, that really don't aid
in the readability of the code, IMHO. Feel free to ignore this, but the
code could be written as:
*
+ inline bool equals(const Symbol* other) const {
+ if (this != NULL && other != NULL) {
+ int len = this->utf8_length();
+ if (len != other->utf8_length()) {
+ return false;
+ }
+ return strncmp((const char*)this->base(), (const char*)other->base(), len) == 0;
+ }
+ return this == other;
+ }
+ inline bool not_equals(const Symbol* other) const {
+ return !this->equals(other);
+ }*
Not related to your change, but why are the Symbol::_body defined as a
jbyte array instead of a char array? That seems to cause unnecessary casts.
Thanks,
StefanK
>
> thanks,
> Calvin
>>>
>>> Thanks,
>>> StefanK
>>>>
>>>> thanks,
>>>> Calvin
>>>>
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>>>
>>>>>> Tests:
>>>>>> JPRT (almost done)
>>>>>> Will do more perf testing after JPRT
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list