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

[Roman Kennke]

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.

>> - 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.

>> - 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...

Cheers,
Roman

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180328/a0724c33/signature.asc>