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

[Daniel D. Daugherty]

Thanks for closing the loop on this part of the review...

I think we're on the same page about what to change.
I have a vague memory of trying to change:

   int buf_size_words = max_locals + active_monitor_count*2;

to:

   int buf_size_words = max_locals + active_monitor_count * 
BasicObjectLock::size();

but it didn't compile for some reason so I went the other way.
I'll take a look one more time before pushing the changeset.
Still doing stress testing so I have time...

Dan


On 3/30/16 11:21 PM, Vladimir Kozlov wrote:
> Sorry for later reply when you send updated webrev already.
>
> You changed code in OSR_migration_begin():
> -  int buf_size_words = max_locals + active_monitor_count*2;
> +  int buf_size_words = max_locals + active_monitor_count * 
> (sizeof(BasicObjectLock) / wordSize);
>
> I am suggesting next:
>
> + int buf_size_words = max_locals + active_monitor_count * 
> BasicObjectLock::size();
>
> Thanks,
> Vladimir
>
> On 3/28/16 4:19 PM, Daniel D. Daugherty 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
>>