[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(S): 8152358 - code and comment cleanups found during the hunt for 8077392
From:       "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date:       2016-03-31 19:45:58
Message-ID: 56FD7E76.7000207 () oracle ! com
[Download RAW message or body]

So this change:

$ diff src/share/vm/runtime/sharedRuntime.cpp{.cr1,}
2956c2956
<   int buf_size_words = max_locals + active_monitor_count * 
(sizeof(BasicObjectLock) / wordSize);
---
 >   int buf_size_words = max_locals + active_monitor_count * 
BasicObjectLock::size();

relative to code review round 1 passes JPRT build and test.
I'll include it in the final changeset for 8152358.

Dan


On 3/31/16 8:37 AM, Daniel D. Daugherty wrote:
> 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
>>>
>
>

[prev in list] [next in list] [prev in thread] [next in thread]