On Mon, 14 Sep 2020 18:44:30 GMT, Daniel D. Daugherty <dcu...@openjdk.org> wrote:
>> @kimbarrett >> >>> src/hotspot/share/runtime/synchronizer.cpp >>> 1548 guarantee(m->object_peek() == NULL, "invariant"); >>> >>> Later: But see previous comment. Some of this might be relevat later though. >>> >>> object_peek() seemed like the wrong operation here. I thought this was >>> attempting to verify that the underlying WeakHandle has been released. But >>> peeking doesn't ensure that. Oh, but we don't actually release() the >>> WeakHandle when we "free" an OM. We're just pushing the OM on the free >>> list. Which means the GC will continue to examine the associated OopStorage >>> entry (and discover it's NULL). So there's some cost to not releasing the >>> WeakHandle when putting an OM on the free list. Of course, it means there >>> won't be any allocations when taking off the free list; I'm not sure which >>> way is better. >>> >>> But this makes me go back and wonder about whether object_peek() should have >>> the _object.is_null() check. After creation it seems like that should never >>> be true. >> >> m->object_peek() == NULL is the right check at that location. om_release() >> is called when we are returning an ObjectMonitor to a free list. At that >> point, >> it should never be associated with an object. > > @kimbarrett > >> src/hotspot/share/runtime/synchronizer.cpp >> >> The old code in chk_in_use_entry seems wrong. It checked for a null >> object() and recorded that as an error. But then it went on and attempted >> to use it as if it was not null. That's been fixed by the change. However, >> the change no longer treats a null as an error. Probably this is because >> it's weak, and so could have become null. But is that really possible for >> an "in use" monitor? > > In the old code, an ObjectMonitor's object field should never be NULL when > that ObjectMonitor is on an in-use list. We'll get the logging message and > then a crash. I used to have guarantee(n->object() != NULL, ...) in there, > but Robbin convinced me that was a waste because we'll just crash on > the use of the NULL pointer and that was good enough. > > As Erik has already explained, now that we use a weak handle, the object > can be GC'ed before the deflater thread comes along and removes the > idle ObjectMonitor from the in-use list. @kimbarrett - I believe I've addressed your comments in this push: https://github.com/openjdk/jdk/pull/135/commits/9fa2bed109d6fe352c610b26ada650226ce9cec4 ------------- PR: https://git.openjdk.java.net/jdk/pull/135
