RFR(S): 8152358 - code and comment cleanups found during the hunt for 8077392

[Carsten Varming]

Dear Dan,

Re my last comment about a potential race between FastHashCode and
fast_exit. Never mind, I just noticed that FastHashCode will inflate the
lock to monitor before installing a hash code.

Carsten

On Tue, Mar 29, 2016 at 11:56 AM, Carsten Varming <varming at gmail.com> wrote:

> Dear Dan,
>
> Generally a very nice clean-up, but I have a few questions.
>
> Quick question: Is it time to remove all those unused perf
> variables: _sync_EmptyNotifications, _sync_Notifications, _sync_SlowEnter, _sync_SlowExit, _sync_SlowNotify, _sync_SlowNotifyAll, _sync_FailedSpins, _sync_SuccessfulSpins, _sync_PrivateA, _sync_PrivateB, _sync_MonInCirculation,
> and _sync_MonScavenged.
>
> synchronizer.cpp:290 you mention that the monitor is either being inflated
> or has already been inflated, but mark->has_monitor() returns false if the
> monitor is being inflated, and this fact is important for the assert in
> line 292 as it would otherwise dereference a NULL pointer. BTW., should the
> if-clause, down to above the return, for the recursive case in fast_exit be
> removed in the product build? It looks like a few dead loads of volatile
> values to me.
>
> There is a comment in synchronizer.cpp:1419 that mentions that it is safe
> to read the displaced mark word of a lock as the thread exiting the lock
> will wait for the object mark word to be different from INFLATING. I don't
> see fast_exit waiting for the mark word to be different from INFLATING in
> the recursive case, so could the mark->displaced_mark_helper() in line 1423
> be reading an arbitrary value on the stack of the thread holding the lock?
> Is this a bug?
>
> I don't understand the comment "Is recursive stack-lock" in
> synchronizer.cpp:303. The recursive case was handled above and ended with a
> return. This should be the non-recursive case. I also don't understand how
> it can be safe to install the old displaced mark word read at the top of
> the method as a different thread could have installed a hashCode into the
> displaced mark word between the read and the CAS into the object mark word.
> Could this gap drop the hashCode, i.e., the value of the java method void
> Object.hashCode() could change for the object?
>
> Carsten
>
> On Mon, Mar 28, 2016 at 7:19 PM, Daniel D. Daugherty <
> daniel.daugherty at oracle.com> wrote:
>
>> On 3/24/16 3:59 PM, Daniel D. Daugherty wrote:
>>
>>> On 3/24/16 3:27 PM, Vladimir Kozlov wrote:
>>>
>>>> In sharedRuntime.cpp you could use BasicObjectLock::size() as you did
>>>> in c1_LIRAssembler_x86.cpp
>>>>
>>>
>>> I'll take a look. I may leave that cleanup to another pass, if
>>> you would be OK with it... I'm nearing the end of my 72 hour
>>> stress test cycle and don't want to repeat it... :-)
>>>
>>
>> Here's the C1 change:
>>
>> $ hg diff src/cpu/x86/vm/c1_LIRAssembler_x86.cpp
>> diff -r b9efb94d011a src/cpu/x86/vm/c1_LIRAssembler_x86.cpp
>> --- a/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp    Mon Mar 07 11:28:06 2016
>> -0800
>> +++ b/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp    Mon Mar 28 16:12:30 2016
>> -0700
>> @@ -312,7 +312,7 @@ void LIR_Assembler::osr_entry() {
>>    Register OSR_buf = osrBufferPointer()->as_pointer_register();
>>    { assert(frame::interpreter_frame_monitor_size() ==
>> BasicObjectLock::size(), "adjust code below");
>>      int monitor_offset = BytesPerWord * method()->max_locals() +
>> -      (2 * BytesPerWord) * (number_of_locks - 1);
>> +      (BasicObjectLock::size() * BytesPerWord) * (number_of_locks - 1);
>>      // SharedRuntime::OSR_migration_begin() packs BasicObjectLocks in
>>      // the OSR buffer using 2 word entries: first the lock and then
>>      // the oop.
>>
>>
>> Here's the existing code in SharedRuntime::OSR_migration_begin():
>>
>>   // Allocate temp buffer, 1 word per local & 2 per active monitor
>>   int buf_size_words = max_locals + active_monitor_count *
>> (sizeof(BasicObjectLock) / wordSize);
>>   intptr_t *buf = NEW_C_HEAP_ARRAY(intptr_t,buf_size_words, mtCode);
>>
>>
>> so is your suggestion to change:
>>
>>   sizeof(BasicObjectLock) / wordSize
>>
>> to:
>>
>>   BasicObjectLock::size()
>>
>> or am I misunderstanding what you mean?
>>
>> Dan
>>
>>
>