On Mon, Jul 25, 2022 at 10:30 PM Stuart Marks <sma...@openjdk.org> wrote: ... > > Hans Boehm wrote: > > > I also have concerns about the use of fullFence here. I believe it should be the case that reachabilityFence guarantees visibility > > of memory operations program-ordered before the reachabilityFence(p) to the Cleaner associated with p. Mutator-collector > > 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. > > > It appears to me that this is addressing an instance of the problem for which I suggested a more general, though also not > > completely elegant, solution in https://docs.google.com/document/d/1yMC4VzZobMQTrVAraMy7xBdWPCJ0p7G5r_43yaqsj-s/edit?usp=sharing . > > Hi Hans, thanks for looking at this. In the prior discussions on reachabilityFence and premature finalization, I don't recall seeing any mention of > 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.) > 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_ > an issue here with the memory model that needs to be addressed. > > 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. > 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 > before deciding what to do. > > 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. > > 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 > 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 > 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 > "outer" object. The `EnumCtx` nested class serves this role here. > > 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 > 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 > 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 > 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. Fully agreed. reachabilityFence use is really orthogonal to the mechanism used for the cleanup action. It also exists for plain References. Technically, you don't even need ReferenceQueues to trigger the problem; there are probably cases in which WeakReferences are polled such that you need reachabilityFence calls.
> > The third issue is memory ordering. For finalization-based objects, the JLS specifies a happens-before edge between the constructor and the > 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, > whose finalizable state is mutable throughout the lifetime of the object.) 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 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. > There is nothing specified for memory ordering edges for Cleaner or > 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 > mechanisms to get the memory order we require. We're using VarHandle::fullFence as kind of a placeholder for this. (We've also considered > empty synchronized blocks, writes/reads to "junk" variables created expressly for this purpose, and other VarHandle fence operations, > but none seem obviously better. I'm sure there are other alternatives we haven't considered.) I'd welcome discussion of the proper alternative. 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 reachabilityFence, it's rather weak anyway. > > 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? > 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 > 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 > address memory ordering issues. There is certainly more that needs to be done though. I clearly agree. > > 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 > 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 > using the existing reachability and memory ordering mechanisms. That's what Brent has done here. We should review and iterate on this and identify > 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. I'm not convinced that's feasible without some spec clarification. Otherwise I would agree. The reachabilityFence spec currently says: "the referenced object is not reclaimable by garbage collection at least until after the invocation of this method." In my opinion, the only meaningful interpretation of this is that the reachabilityFence call happens before any Reference to the argument object is enqueued. Preventing "garbage collection" per se isn't ever a goal. (And in the case of finalizers, it happens much later than finalization, 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. 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 effect we might largely be able to get rid of 12.6.2, which would be a huge win. 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 the given reference remains strongly reachable". Presumably this implies, via the Reference spec, that References are not enqueued until later. Even there, it's unclear to me what "later" means, except in terms of happens-before. AFAICT, implementations actually do comply with my reading, though an OpenJDK expert should confirm. We won't enqueue a Reference to x, where 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. The safepoint synchronization must ensure that the safepoint happens-before the GC decision to enqueue, which will then happen-before the actual enqueuing. Enforcing this ordering is cheap, and probably necessary for GC correctness anyway. So in implementation terms, I think my interpretation is safe. 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 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, so I don't think we have a guarantee they help, except by an assumption that also implies they're not needed. > > 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. > Or maybe some alternative path forward might emerge. 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. Even if we find a better mechanism for most case, I expect that reachabilityFence will still make sense in corner cases. Hans > > ------------- > > PR: https://git.openjdk.org/jdk/pull/8311
