On Fri, 11 Apr 2025 08:51:56 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> I think it would be "easier" to shift `@IntrinsicCandidate` to a private >> `Reference.getImpl/get0` pair of methods, and intrinsify one of those >> instead. Pretty much like current `clear` and `refersTo` are doing it now. >> And it would be in line with what Kim is doing in this PR anyway. > > 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 changed here is the ordinary definition of that method. It needs to remain virtual, so we don't want to make it directly native, because of the above mentioned C2 issue. So we make it call a private native helper function. This method is not applicable to either PhantomReference or FinalReference, as they provide their own implementations, so the native implementation only needs to support weak strength. For Reference::get we don't need a separate virtual getImpl. The reasons for the xxxImpl methods for refersTo and clear don't apply here. We could move the intrinsification from Reference::get to to private native Reference::get0 (without an intervening Impl), to more closely correspond to refersTo and clear (where the intrinsics are the associated private native methods). But that would require renaming the intrinsic id for consistency with the name of the intrinsified method, and other renamings as well. I'd considered doing that initially, but decided that was a bunch of code churn with no benefit. So in summary, I don't think any changes to this PR are needed based on this comment thread (at least so far). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24315#discussion_r2039137923
