RFR(T): 8251543: add mention of INFLATING() to share/oops/markWord.hpp header comment

[Daniel D. Daugherty]

On 8/16/20 10:42 PM, David Holmes wrote:
> Hi Dan,
>
> On 16/08/2020 12:04 am, Daniel D. Daugherty wrote:
>> Tap, tap, tap... is this thing working? Any takers for a trivial review?
>
> I had a long weekend. :(

No worries.


> I see you pushed this but it isn't really what I was looking for. 
> Specifically I wanted the fact that an all-zero header meant 
> "inflating" would be clear from the added text. At the moment if I see:
>
> //    [ptr             | 00]  locked             ptr points to real 
> header on stack
>
> //    [header      | 0 | 01]  unlocked           regular object header
>
> //    [ptr             | 10]  monitor            inflated lock (header 
> is wapped out)
>
> //    [ptr             | 11]  marked             used to mark an object
>
> then it appears that checking for 00 in lsb is sufficient to identify 
> a locked object. But that isn't true because all zeros means 
> inflating. I had hoped to see that explicitly added e.g.
>
> //    [0      ...    0| 00]  inflating             inflation in progress
>
> then your new text. At a minimum I would have liked the new text to 
> actually state that the INFLATING value is all zeroes.

When I first wrote the comment, I mentioned that INFLATING was all zeroes.
Later I went back and reread the comment and guessed that you wouldn't like
that I mentioned that level of detail. Kind of like when I add comments that
detail underlying memory barrier ops and you've asked me to be less 
specific.

I can file a new bug to add the following to that "table":

//    [0      ...    0| 00]  inflating             inflation in progress


> Sorry that my expectations were not clear enough.

No worries. I probably should have waited to hear from you since you
asked for the comment.

Dan


>
> David
> -----
>
>> Dan
>>
>>
>> On 8/13/20 5:16 PM, Daniel D. Daugherty wrote:
>>> In a previous cleanup bug, we normalized 
>>> src/share/runtime/synchronizer.cpp to
>>> use "stack-lock" instead of "stack lock" and other variants. I'm 
>>> going to change
>>> the two "stack lock" uses to "stack-lock" below.
>>>
>>> Dan
>>>
>>>
>>> On 8/13/20 10:58 AM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I have a trivial comment addition to 
>>>> src/hotspot/share/oops/markWord.hpp.
>>>>
>>>> Here's the bug:
>>>>
>>>>     JDK-8251543 add mention of INFLATING() to 
>>>> share/oops/markWord.hpp header comment
>>>>     https://bugs.openjdk.java.net/browse/JDK-8251543
>>>>
>>>> And here's the proposed fix:
>>>>
>>>> $ hg diff
>>>> diff -r 73778bfbffe3 src/hotspot/share/oops/markWord.hpp
>>>> --- a/src/hotspot/share/oops/markWord.hpp    Thu Aug 13 10:02:35 
>>>> 2020 -0400
>>>> +++ b/src/hotspot/share/oops/markWord.hpp    Thu Aug 13 10:51:44 
>>>> 2020 -0400
>>>> @@ -86,6 +86,10 @@
>>>>  //    [ptr             | 11]  marked             used to mark an 
>>>> object
>>>>  //
>>>>  //    We assume that stack/thread pointers have the lowest two 
>>>> bits cleared.
>>>> +//
>>>> +//  - INFLATING() is a distinguished markword value that is used when
>>>> +//    inflating an existing stack lock into an ObjectMonitor. See 
>>>> below
>>>> +//    for is_being_inflated() and INFLATING().
>>>>
>>>>  class BasicLock;
>>>>  class ObjectMonitor;
>>>> @@ -226,7 +230,7 @@
>>>>    bool is_being_inflated() const { return (value() == 0); }
>>>>
>>>>    // Distinguished markword value - used when inflating over
>>>> -  // an existing stacklock.  0 indicates the markword is "BUSY".
>>>> +  // an existing stack lock.  0 indicates the markword is "BUSY".
>>>>    // Lockword mutators that use a LD...CAS idiom should always
>>>>    // check for and avoid overwriting a 0 value installed by some
>>>>    // other thread.  (They should spin or block instead. The 0 value
>>>>
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> Dan
>>>
>>