RFR: JDK-8199781: Don't use naked == for comparing oops

[coleen.phillimore at oracle.com]

On 3/20/18 12:06 PM, Stefan Karlsson wrote:
> Hi Roman,
>
> On 2018-03-19 19:32, Roman Kennke wrote:
>> GCs might need to handle object comparison specially, for example, when
>> a concurrent GC allows both from- and to-space copies of an object to be
>> in use. In this case, object equality cannot be expressed by direct
>> address comparsion, but must also take potential different versions of
>> an object into account.
>>
>> This adds a method oopDesc::equals(oop o1, oop o2), which goes through
>> the Access API to compare objects. Default impl does ==
>>
>> It is based on our finds from Shenandoah. We do, in fact, have some
>> extra code in oopsHierarchy.hpp to detect naked oop==oop (with
>> CheckUnhandledOops enabled), but it requires that code that does really
>> want naked == (e.g. GCs) to use another method oopDesc::unsafe_equals()
>> instead, and code that wants to use == and assert that the comparison is
>> safe (e.g. both objects are already in to-space) use
>> oopDesc::safe_equals(). Not sure if you want that?
>>
>> Also, I'd have preferred to mark equals() as inline in oop.hpp, but that
>> pulls in a rats nest of dependencies problems. I hope the way I did it
>> is acceptable?
>
>
> We have actively been working on cleaning up our include dependencies, 
> so this should be dealt with properly before this patch can be 
> accepted. All files that use oopDesc::equals need to include 
> oop.inline.hpp. The usages of oopDesc::equals in .hpp needs to move to 
> either .cpp files or .inline.hpp files.

Stefan, Thank you for noticing this.  If I click through the files, it 
looks like there aren't any new #include oop.inline.hpp in header files, 
but I see there are calls to oopDesc::equals() from some .hpp files, so 
I assume they have transitively included oop.inline.hpp in order to 
compile.  e.g.

http://cr.openjdk.java.net/~rkennke/JDK-8199781/webrev.00/src/hotspot/share/runtime/handles.hpp.udiff.html

I have a patch that I'm testing to remove many of the .inline.hpp files 
from header files with:

https://bugs.openjdk.java.net/browse/JDK-8199809

When it's a little closer, I'll give you the patch to test this on.

After this change, I'm going to declare being done for now.

Most of this change below looks okay to me, except that calls to 
oopDesc::equals should be moved to .inline.hpp.   I don't think that the 
jvm is modularly structured well enough to not need every .inline.hpp or 
.cpp file to include oop.inline.hpp though, which is unfortunate also.

Thanks,
Coleen


>
> See the files section on this page:
> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
>
> Currently, you need to include access.inline.hpp to use the Access 
> API. I wish this wasn't the case, and that we somehow hide the GC 
> internal dependencies, so that the call sites didn't have to bring all 
> those dependencies. Erik Ö used to have a facility to generate the 
> runtime dispatched functions out-of-line. Maybe if we build upon this, 
> we could get rid of the need to include access.inline.hpp when using 
> the Access API, and oopDesc::equals could be placed in 
> oopDesc::equals. However, we don't have this in place (yet) so we need 
> to play along with the set rules for .inline.hpp files.
>
> Thanks,
> StefanK
>
>>
>> There's one little thing in growableArray.hpp: in order to do the right
>> thing for GrowableArray<oop> I needed to provide a special templated
>> comparison function, with a specialized version for oop.
>>
>> It also overloads the == operator in Handle to do the right thing.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rkennke/JDK-8199781/webrev.00/
>>
>> Testing: built fastdebug/release, tier1_runtime, tier1_gc
>>
>> Please review!
>>
>> Thanks, Roman
>>