Bug ID: JDK-8236273 clarify use and role of clear() and Recycle() in ObjectMonitor

Type: Bug
Component: hotspot
Sub-Component: runtime
Affected Version: 15

Priority: P4
Status: Resolved
Resolution: Duplicate
OS: generic
CPU: generic

Submitted: 2019-12-19
Updated: 2020-11-11
Resolved: 2020-11-11

David sent the following query:

On 12/16/19 11:56 PM, David Holmes wrote:
> Hi Dan,
>
> Given that we always call
>
> m->Recycle()
>
> after
>
> ObjectMonitor * m = om_alloc(..);
>
> why do we also call Recycle() when we are putting an OM back into a freelist? Seems completely unnecessary to me.
>
> Also wondering if there is any particular reason why Recycle() doesn't set _Responsible  = NULL ? Seems to me Recycle() is not completely clear on what its job is.
>
> I'm trying to understand all the nuances here as part of the JavaObjectMonitor work - which progresses very very slowly.
>
> Thanks,
> David

JDK-8253064 has been integrated. Closing this bug as a duplicate.
11-11-2020
Erik and Dan's work on this bug: JDK-8253064 monitor list simplifications and getting rid of TSM will delete clear() and Recycle() so this bug can be closed as a duplicate if/when JDK-8253064 gets integrated.
05-11-2020
David sent one more set of new observations: On 12/18/19 9:29 PM, David Holmes wrote: > Hi Dan, > > I'm seeing a use for Recycle (better named prepare) and clear, as complementary, even though there is a lot of overlap. Recycle ensures a monitor is ready for use (for me I allocate the Java Monitor); clear clears the remaining fields (header,object,monitor) of a quiescent OM. > > Seems to me that in this code sequence > > ObjectMonitor* m = om_alloc(self); > // prepare m for installation - set monitor to initial state > m->Recycle(); > m->set_header(mark); > m->set_object(object); > > if (object->cas_set_mark(markWord::encode(m), mark) != mark) { > m->set_header(markWord::zero()); > m->set_object(NULL); > m->Recycle(); > om_release(self, m, true); > > the three steps before release: > > m->set_header(markWord::zero()); > m->set_object(NULL); > m->Recycle(); > > should actually be: > > m->clear(); > > Cheers, > David
19-12-2019
David sent the following reply: On 12/17/19 5:27 PM, David Holmes wrote: > Hi Dan, > > On 18/12/2019 6:28 am, Daniel D. Daugherty wrote: >> Hi David, <snip> >> I suspect that we only really need to clear those fields in >> one place. I'll have to take a look at the placement of the >> clear() calls and determine if there is a good reason to >> separate what clear() does from what Recycle() does. > > I commented out the Recycle() calls when putting a OM back into a freelist (in regular hotspot) and it didn't crash :) > > For Java ObjectMonitor I actually need two functions to set the OM state correctly after om_alloc and before om_release. So I'll probably repurpose Recycle and clear. > > Cheers, > David
19-12-2019
I sent the following reply: On 12/17/19 3:28 PM, Daniel D. Daugherty wrote: > Hi David, > > This is definitely an area where things are confused! We have: > > ObjectMonitor::Recycle() > ObjectMonitor::clear() > direct clearing/setting of various fields > > Indeed we call Recycle() from four places: > > 1) In the "// 2: try to allocate from the global g_free_list" block > where we call it on each ObjectMonitor we take off the global free > list. > > 2) In the "// CASE: stack-locked" inflate() block after we call > om_alloc(). > > 3) In the "// CASE: neutral" nflate() block after we call > om_alloc() and > > 4) we call it again if the: > > (object->cas_set_mark(markWord::encode(m), mark) != mark) > > in the "// CASE: neutral" inflate() block fails, but we don't call > it if "// CASE: stack-locked" fails its CAS... > > Comments along these lines have come up during various reviews > and I really need to file a bug to clear up this mess. Do you > mind if I quote your email in the new bug? > > I suspect that we only really need to clear those fields in > one place. I'll have to take a look at the placement of the > clear() calls and determine if there is a good reason to > separate what clear() does from what Recycle() does. > > Dan
19-12-2019