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

[Carsten Varming]

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
>
>