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

[Coleen Phillimore]

Hi Calvin, and fellow JVM engineers,

I prefer a modification of your first version of this change much better.

I really don't like this...  It feels very unsafe to me.  I don't know 
how to run any tools to make sure I don't break this!  Honestly this 
seems wrong and there are too many places that compare Symbols even 
though the changeset is smaller.

We triage bugs in the runtime group weekly with SQE.  This change will 
cause bugs that have various symptoms and will be hard to trace to this 
root cause.  The bugs will mostly land in the Runtime component of the 
JVM because in fact, the Symbol class is mostly used by the runtime 
component of the JVM.  In addition, running internal tools to find these 
errors *monthly* is too late and running them individually adds overhead 
and friction to making changes in the JVM.  More overhead is the last 
thing we need!

Having to use ->equals() is clunky too.  For better or worse, the JVM is 
written in C++ which has operator overloading for these purposes.  
Modern C++ programming already avoids raw pointers in favore of smart 
pointers!   The JVM code has historically avoided raw metadata pointers, 
first because of PermGen but now because the values pointed to have 
semantics that we want to encapsulate.  I admit that it was nice using 
raw pointers and brought all of us back to a simpler day but they're not 
safe in general for this sort of system software.

In the JVM code, we have Handles:

1. oops => Handle because they may move with garbage collection.
2. Method* => methodHandle because they may get deallocated with class 
redefinition (same for Klass eventually)
3. Symbol* => SymbolHandle because pointer equality isn't sufficient to 
ensure equality

The other objection for Calvin's first change was that it's a lot of 
code changed.  But there's a lot of other large code changes going 
forward at this time.  This is the simplest of large changes, ie. simple 
renaming.   This feature is needed for others going forward to support 
our important customers.  This amount of code change is justified.

Embedded in this mail, if you've read this far, is a suggestion to 
rename SymbolRef (a name I hate) to SymbolHandle.  Because that's what 
it now is.

Thanks,
Coleen


On 4/15/15, 3:56 PM, 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/
>
> Tests:
>     JPRT (almost done)
>     Will do more perf testing after JPRT
>
> thanks,
> Calvin