RFR: 8352565: Add native method implementation of Reference.get() [v4]

[Aleksey Shipilev]

On Fri, 11 Apr 2025 09:09:05 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> On a second thought, I think we should do this shift before this PR, so that it is cleanly backportable. This bug messes with concurrent GC invariants, so it would be nice to fix it in current LTSes.
>
> As far as I can tell, intrinsic selection only applies when the call target is
> exactly the intrinsically attributed method. (Possibly after optimizations
> that lead to a call to that specific method.) And that's obviously necessary
> for correctness of applying intrinsics.
> 
> The documentation for `@IntrinsicCandidate` suggests it is "better to make
> intrinsics be static methods", but non-static methods are certainly permitted,
> and I've found several. In particular, the method in question,
> `Reference::get`, has been a virtual intrinsic with at least three
> (non-intrinsic) overrides (SoftReference, PhantomReference, and
> FinalReference) for a long time.
> 
> I did a bit of experimenting, and as far as I can tell, everything works as
> expected.  Reference::get is already registered as having
> vmIntrinsics::_Reference_get as it's intrinsic_id, and that intrinsic id is
> handled by the various code processors, such as being in the dispatch table
> that @vnkozlov refreenced.
> 
> All of Reference::get, Reference::refersTo, and Reference::clear are subtly
> different in the details of how they are implemented and intrinsified, because
> there are differences among them.
> 
> refersTo is final, but needs different implementations for Reference and
> PhantomReference. That distinction is handled via nonpublic virtual
> refersToImpl. And those are implemented in terms of separate intrinsified
> private native methods because we found that C2 handling of intrinsified
> virtual native methods had problems.  If not for that issue, the refersToImpl
> methods would be intrinsified virtual native methods.
> 
> Reference::clear also needs clearImpl, but for a different reason. There are
> places where we want to directly call the implementation of Reference::clear,
> ignoring any overrides by derived application classes. That is accomplished by
> calling the virtual clearImpl, which has implementations in Reference and
> PhantomReference. And the clearImpls are implemented in terms of separate
> intrinsified private native methods because of the above mentioned C2 issue.
> 
> The native implementations for clear call the same JVM_ReferenceClear helper
> function, which uses unknown_referent_no_keepalive to handle the distinction
> between weak and phantom strengths.  If we just had the native implementation
> we could drop the PhantomReference override.  But this operation also has
> intrinsic support, and the intrinsic makes use of the known strength.
> 
> Reference::get already has long-standing intrinsic support.  The only thing
> being ch...

I think I understand the intrinsic matchers machinery better now: it indeed binds intrinsics to concrete methods. So virtual overrides are supposed to have no difference. Also, I tried a few examples where I thought it should fail, and it does not. I added some diagnostic checks around interpreter, and they do not fail:


diff --git a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
index bdc2ca908bd..255fa6443af 100644
--- a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
+++ b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
@@ -652,6 +652,21 @@ address TemplateInterpreterGenerator::generate_Reference_get_entry(void) {
   // rdx: scratch
   // rdi: scratch
 
+  if (UseNewCode) {
+    Label L_good;
+    __ push(rscratch1);
+    __ push(rscratch2);
+
+    __ load_klass(rscratch1, rax, rscratch2);
+    __ cmpb(Address(rscratch1, in_bytes(InstanceKlass::reference_type_offset())), REF_PHANTOM);
+    __ jccb(Assembler::notEqual, L_good);
+    __ stop("PHANTOM REFERENCE CALLED WITH INTERPRETER REFERENCE.GET INTRINSIC");
+    __ bind(L_good);
+
+    __ pop(rscratch2);
+    __ pop(rscratch1);
+  }
+
   // Load the value of the referent field.
   const Address field_address(rax, referent_offset);
   __ load_heap_oop(rax, field_address, /*tmp1*/ rbx, ON_WEAK_OOP_REF);


I tried C2 IR test, and it works fine: we return constant `0` for `PhantomReferences`, like we would expect. I am going to RFR that test separately.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24315#discussion_r2039300718