<div dir="ltr"><div dir="ltr"><br></div><br>On Mon, Jul 25, 2022 at 10:30 PM Stuart Marks <<a href="mailto:smarks@openjdk.org" target="_blank">smarks@openjdk.org</a>> wrote:<br>...<br>><br>> Hans Boehm wrote:<br>><br>> > I also have concerns about the use of fullFence here. I believe it should be the case that reachabilityFence guarantees visibility<div>> > of memory operations program-ordered before the reachabilityFence(p) to the Cleaner associated with p. Mutator-collector</div><div>> > synchronization should normally ensure that. On rereading, it also appears to me that the current reachabilityFence() documentation does not make that as clear as it should.<br>><br>> > It appears to me that this is addressing an instance of the problem for which I suggested a more general, though also not</div><div>> > completely elegant, solution in <a href="https://docs.google.com/document/d/1yMC4VzZobMQTrVAraMy7xBdWPCJ0p7G5r_43yaqsj-s/edit?usp=sharing" target="_blank">https://docs.google.com/document/d/1yMC4VzZobMQTrVAraMy7xBdWPCJ0p7G5r_43yaqsj-s/edit?usp=sharing</a> .<br>><br>> Hi Hans, thanks for looking at this. In the prior discussions on reachabilityFence and premature finalization, I don't recall seeing any mention of</div><div>> memory model issues. To my mind the discussions here are the first mention of them. (Of course it's possible I could have missed something.)</div><div>> The memory model is quite subtle and it's difficult to be sure of anything. However, as time goes on we've become more confident that there _IS_</div><div>> an issue here with the memory model that needs to be addressed.<br>><br>> Then there is a the question of what to do about it. One possibility is to add some memory ordering semantics into reachabilityFence(p), as you suggest.</div><div>> As convenient as it might seem, it's not obvious to me that we want reachability fused with memory order. At least we should discuss them separately</div><div>> before deciding what to do.<br>><br>> This PR seems to be on a long journey :-) so let me describe some of the background and where I think this ought to go.<br>><br>> First, like most PRs, this started off as an effort to make some code changes. In this case it's part of Brent's (@bchristi-git) effort to convert finalizers</div><div>> in the JDK to Cleaners. This is related to discovering coding patterns for how to do this effectively. For example, the entire object's state is available</div><div>> to the finalizer. But in order to convert this to a Cleaner, the state available to the cleaning action needs to be refactored into a separate object from the</div><div>> "outer" object. The `EnumCtx` nested class serves this role here.<br>><br>> Second, we're trying to address the reachability issues. You (Hans) have been writing and speaking about this for some time, mostly in the context of</div><div>> finalization. We in the JDK group haven't prioritized fixing this issue with respect to finalization (since it's old and deprecated and nobody should be</div><div>> using it, yeah right). Now that we're converting things to use Cleaner, which has the same issues, we're forced to confront them. Our working position</div><div>> is that there needs to be a reachabilityFence within a try/finally block in the "right" places; determining the definition of "right" is one of the goals here.</div><div>Fully agreed. reachabilityFence use is really orthogonal to the mechanism used for the cleanup action. It also exists for plain References. Technically,</div><div>you don't even need ReferenceQueues to trigger the problem; there are probably cases in which WeakReferences are polled such that you need</div><div>reachabilityFence calls.</div><div><br>><br>> The third issue is memory ordering. For finalization-based objects, the JLS specifies a happens-before edge between the constructor and the</div><div>> finalizer. (I think this works for objects whose finalizable state is fully initialized in the constructor. But it doesn't work for objects, like this one,</div><div>> whose finalizable state is mutable throughout the lifetime of the object.)</div><div>My recollection is that was added as a very minimal guarantee, that was not intended to suffice in general. Even if the state is fully initialized</div><div>in the constructor, things will usually still break if the final method call that uses the state does not happen before the execution of the Cleaner.</div><div><br></div><div>> There is nothing specified for memory ordering edges for Cleaner or</div><div>> java.lang.ref references at all that I can see. Given the lack of such specifications, we're faced with using the right low-level memory ordering</div><div>> mechanisms to get the memory order we require. We're using VarHandle::fullFence as kind of a placeholder for this. (We've also considered</div><div>> empty synchronized blocks, writes/reads to "junk" variables created expressly for this purpose, and other VarHandle fence operations,</div><div>> but none seem obviously better. I'm sure there are other alternatives we haven't considered.) I'd welcome discussion of the proper alternative.</div><div>I think the intent was that JLS 12.6.2 still applies, though that section was always stated in terms of finalizers. And in the absence of</div><div>reachabilityFence, it's rather weak anyway.</div><div><br>><br>> The fourth issue is, do we really want every programmer who uses Cleaner to have to figure out all this reachabilityFence and VarHandle fence stuff?</div><div>> Of course not. It would be nice to have some higher-level construct (such as a language change like the "Reachability Revisited" proposal), or possibly</div><div>> to create some library APIs to facilitate this. At a minimum, I think we need to adjust various specifications like Cleaner and java.lang.ref to</div><div>> address memory ordering issues. There is certainly more that needs to be done though.</div><div>I clearly agree.</div><div><br>><br>> The difficulty with trying to come up with language changes or library APIs is that I don't think we understand this problem well enough to define</div><div>> what those mechanisms are actually supposed to do. So before we get to that point, I think we should see attempt to write down a correct solution</div><div>> using the existing reachability and memory ordering mechanisms. That's what Brent has done here. We should review and iterate on this and identify</div><div>> the places where the specs need to change, and arrive at a point where the we believe the code, even if ugly and inconvenient, is correct under that spec.</div><div>I'm not convinced that's feasible without some spec clarification. Otherwise I would agree.</div><div><br></div><div>The reachabilityFence spec currently says: "the referenced object is not reclaimable by garbage collection at least until after the invocation of this method."</div><div><br></div><div>In my opinion, the only meaningful interpretation of this is that the reachabilityFence call happens before any Reference to the argument object is enqueued.</div><div>Preventing "garbage collection" per se isn't ever a goal. (And in the case of finalizers, it happens much later than finalization,</div><div>so it's not the right notion anyway.) And the only sense of "after" that makes sense in such contexts is the inverse of the happens-before relation.</div><div>In retrospect, this interpretation is definitely a stretch, and the spec should be much clearer. And I suspect that if we had a clearer statement to this</div><div>effect we might largely be able to get rid of 12.6.2, which would be a huge win.</div><div><br></div><div>It sounds like you're applying an alternative reading that largely ignores the clause I quoted, and simply goes by "Ensures that the object referenced by</div><div>the given reference remains strongly reachable". Presumably this implies, via the Reference spec, that References are not enqueued until later. Even</div><div>there, it's unclear to me what "later" means, except in terms of happens-before.</div><div><br></div><div>AFAICT, implementations actually do comply with my reading, though an OpenJDK expert should confirm. We won't enqueue a Reference to x, where</div><div>x was last referenced by reachabilityFence(x) in thread T, unless there is an intervening GC that result in T at a safe-point past the reachabilityFence.</div><div>The safepoint synchronization must ensure that the safepoint happens-before the GC decision to enqueue, which will then happen-before the actual</div><div>enqueuing. Enforcing this ordering is cheap, and probably necessary for GC correctness anyway. So in implementation terms, I think my interpretation is safe.</div><div><br></div><div>In contrast, putting in explicit fullFences is clearly horribly expensive, much more so than a reachabilityFences. And they only make sense if they occur both at the use</div><div>sites and in the Cleaner. And without my "happens-before" interpretation, we technically don't know that use site one happens before the one in the Cleaner,</div><div>so I don't think we have a guarantee they help, except by an assumption that also implies they're not needed.</div><div><br></div><div>><br>> Maybe at that point we can contemplate a higher-level approach such as a language mechanism or a library API (or both), but I don't think we're quite there yet.</div><div>> Or maybe some alternative path forward might emerge.</div><div>We share that hope. None of the current options look beautiful to me either. But I think it would also be useful to agree that reachabilityFence implies memory ordering.</div><div>Even if we find a better mechanism for most case, I expect that reachabilityFence will still make sense in corner cases.</div><div><br></div><div>Hans<br>><br>> -------------<br>><br>> PR: <a href="https://git.openjdk.org/jdk/pull/8311" target="_blank">https://git.openjdk.org/jdk/pull/8311</a></div></div>