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

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 notequals()) 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 notequals: - if (name != vmSymbols::objectinitializername()) { + if (name->notequals(vmSymbols::objectinitializername())) { instead of just: + if (!name->equals(vmSymbols::objectinitializername())) { We think that it's clearer to have notequals() than using the ! as in the above. OK. IMHO, negating boolean expressions with ! is a common construct so I'm not sure notequals is clearer. http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/classfile/systemDictionary.cpp.udiff.html ! assert(parsedname->notequals(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::throwmsg(THREADANDLOCATION, vmSymbols::javalangSecurityException(), message); } if (!HASPENDINGEXCEPTION) { ! assert(parsedname != NULL, "Sanity"); ! assert(classname == NULL || classname == parsedname, "name mismatch"); ! assert(parsedname->notequals(NULL), "Sanity"); ! assert(classname == NULL || classname->equals(parsedname), "name mismatch"); // Verification prevents us from creating names with dots in them, this // asserts that that's the case. assert(isinternalformat(parsedname), "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: classname == NULL parsedname != NULL or 2) strictly use the (not)equals function when checking for NULL: classname->equals(NULL) parsedname->notequals(NULL) or even: classname->equals(NULL) !parsedname->equals(NULL)) but not what you have today, where you mix the two ways to NULL check classname == NULL parsedname->notequals(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->notequals(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->identityhash != other->identityhash) { + return false; + } Is this correct? The current code sets up identityhash with os::random, so it seems like to Symbols with equal contents would most likely going to get different identityhash 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:

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