RFR (2nd round) 8071627: Code refactoring to override == operator of Symbol*
coleen.phillimore at oracle.com
Wed Apr 22 21:11:39 UTC 2015
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
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.
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
> It has a much smaller changeset than the first version.
> 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/
> JPRT (almost done)
> Will do more perf testing after JPRT
More information about the hotspot-dev