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

[Erik Österlund]

Hi Roman,

On 2018-03-28 17:23, Roman Kennke wrote:
> Am 28.03.2018 um 17:04 schrieb Erik Österlund:
>> Hi Roman,
>>
>> On 2018-03-28 16:03, Roman Kennke wrote:
>>> Hi Erik,
>>>
>>> Thanks for this work!
>>>
>>> without having looked too deeply at the code (it might be in vain
>>> anyway, what with all the template voodoo stuff going on :-P), I have a
>>> few conceptual questions:
>>>
>>> - For the issue at hand (replacing naked == with oopDesc::equals() or
>>> such), wouldn't it have been enough to mark it with the
>>> INTERNAL_BT_TO_SPACE_INVARIANT decorator, to get inlining for
>>> non-Shenandoah (or more generally, non-to-space-invariant) builds. ?
>> I have done that as well to get inlining for non-to-space-invariant
>> builds. But obviously, by moving e.g. Handles::operator == to the cpp
>> file, and oopDesc::equals to the cpp file, you broke inlining anyway. To
>> get the inlining back, choices were considering comparing two naked oops
>> as not trivial and hence forcing all transitive dependencies to code
>> that compares two naked oops to move to .inline.hpp files, or to make it
>> allowed from .hpp files. This is my solution for allowing existing
>> callsites that compare naked oops in .hpp files (because it was IMO
>> rightfully considered trivial), to remain in .hpp files, and preserve
>> all inlining. Therefore, only looking for that build-time decorator
>> would not have been enough to make this legally allowed from .hpp files,
>> because the very logic for folding away the non-to-space-invariant path
>> was in access.inline.hpp.There must also exist a path for the
>> non-to-space-invariant build from hpp files only to get to get to the GC
>> backend implementation, which relies on .inline.hpp file support. That
>> is why I have shuffled things around to be able to reach all the way up
>> until but not including barrier resolution through the hpp files, and be
>> able to finish the rest of the path in access.cpp for selected operations.
> Ok. But my last patch moved all uses of == into .inline.hpp or .cpp
> files, out of .hpp to be sure. So that should have solved that, and not
> break inlining.

I'm afraid not. You called oopDesc::equals (that had its definition in 
oop.inline.hpp) from GrowableArray::safe_equals, defined in 
growableArray.hpp, hence violating our include dependency rules. You 
mentioned in the review you did not know how to fix that, as respecting 
the include dependencies properly would cause a mess when expanding the 
transitive include dependencies. My patch allows it to stay there 
without violating our include dependency rules. And obviously, moving 
things to cpp files is prone to breaking inlining in the general case.

>>> - What is the advantage of this refactoring? It is a refactoring, right?
>>> or do we get new features here?
>> This is a refactoring that allows using Access<>::equals() from old
>> header files that have been included all over the place, that have used
>> raw oop comparisons in .hpp files in the past. One alternative is moving
>> the caller of Access<>::equals to cpp files (and I heard objections to
>> that from both Andrew Haley and John Rose, arguing that this should be a
>> fast operation, and losing the inlining purely because of file
>> dependency technicalities would be a shame). Another alternative is
>> moving these callsites out to .inline.hpp files, and then expanding the
>> transitive closure of required .inline.hpp dependency changes that
>> creates. This is my attempt at making everyone happy - maximized
>> inlining with or without to-space-invariant GCs in the build, without
>> violating transitive .inline.hpp include dependency rules, and without
>> having to move everything that ever transitively depends on comparing
>> two oops to .inline.hpp files.
> Sounds ok. I'll give the change a thorough look later today.

Thank you for having a look at this.

>>> - So now we can use the Access API by only including access.hpp. Fine.
>>> But this comes at the cost of having actual code in .hpp files? I am not
>>> sure about Hotspot style, but I try to avoid actual code in .hpp files
>>> whenever I can, except *maybe* for the most trivial of stuff.
>> For now, this is only intended for Access<>::equals to solve your
>> problem at hand in this review: not violating transitive .inline.hpp
>> dependencies, while preserving inlining all the way for
>> non-to-space-invariant builds, and at the same time injecting variation
>> points for Shenandoah without any GC backend .inline.hpp dependencies
>> leaking into .hpp files. While this mechanism can be extended for more
>> operations, there is currently no need for it.
>>
>> About code style of calling "actual code" from .hpp files, I agree that
>> it is a kind of gray area, where it is preferable to move non-trivial
>> things to .inline.hpp files. Comparing naked oops has been considered
>> trivial in the past by engineers, (despite calling oop::operator ==()
>> when CHECK_UNHANDLED_OOPS is enabled), or now by calling
>> oopDesc::equals, and I can see why. This patch allows existing code that
>> considered oop1 == oop2 to be trivial enough to be in a header file to
>> continue doing so, without violating transitive include dependency rules.
> Ok. Thanks for this support! I'll review it later today, need to rush
> out for a bit...

No problem Roman, I am happy to help. Thank you for reviewing.

/Erik

> Cheers,
> Roman
>