RFR: JDK-8234507 [lworld] Macro ASM version of access_value_copy

[Frederic Parain]

David,

Thank you for making these changes.
Looks good to me.

Regards,

Fred


> On Nov 22, 2019, at 08:03, David Simms <david.simms at oracle.com> wrote:
> 
> 
> Thanks for the review, I know it is not the easiest code to pick through, thanks for taking the time...
> 
> Updated webrev1: http://cr.openjdk.java.net/~dsimms/valhalla/8234507/webrev1/
> 
> All comments applied, see below for details...
> 
> On 21/11/19 5:20 PM, Frederic Parain wrote:
>> David,
>> 
>> Thank you for this work, it was huge but it seems you tackled it nicely.
>> Nice refactoring of the allocation code.
>> And the performance numbers sound great.
>> 
>> Here’s my feedback on your webrev:
>> 
>> 
>> src/hotspot/cpu/x86/gc/shared/barrierSetAssembler_x86.cpp
>> 
>>   No comment
>> 
>> src/hotspot/cpu/x86/gc/shared/barrierSetAssembler_x86.hpp
>> 
>>   No comment
>> 
>> src/hotspot/cpu/x86/interp_masm_x86.cpp
>> 
>>   No comment
>> 
>> src/hotspot/cpu/x86/interp_masm_x86.hpp
>> 
>>   line 227: does the method really need to be virtual?
> 
> Nope, removed
> 
>> 
>> src/hotspot/cpu/x86/macroAssembler_x86.cpp
>> 
>>   line 4520: using temp_reg instead of obj for the temp register of test_klass_is_empty_value()
>>              would make the code easier to read (just my opinion)
> You are correct, done
>> 
>> src/hotspot/cpu/x86/macroAssembler_x86.hpp
>> 
>>   line 107: I don’t get the comment (seems to be a bad copy/paste from line 574)
>>   line 551: does the method really need to be virtual?
> 
> 107: you are right, looks like when I rearranged where method decls went
> 
> 551: nope, removed
> 
>> src/hotspot/cpu/x86/templateTable_x86.cpp
>> 
>>    line 4339: the template of defaultvalue could be simplified by using the new get_empty_value_oop()
>>    line 3704: qputfield could be re-written to use access_value_copy() and avoid the runtime call
>>               (this could be delayed to another CR)
>>    line 3795: Label slow_path is not used
> 
> 4339: Done
> 
> 3704: Yeap, but deferred for another CR for now (JDK-8234632 for putfield and JDK-8234633 for aaload/aastore)
> 
> 3795: Label removed
> 
>> 
>> src/hotspot/share/gc/shared/collectedHeap.hpp
>> 
>> src/hotspot/share/gc/shared/collectedHeap.inline.hpp
>> 
>>   No comment
>> 
>> src/hotspot/share/gc/shared/memAllocator.cpp
>> 
>>   No comment
>> 
>> src/hotspot/share/gc/shared/memAllocator.hpp
>> 
>>   No comment
>> 
>> src/hotspot/share/oops/instanceKlass.cpp
>> 
>>   line 508: [style] add empty line?
>> 
>> src/hotspot/share/oops/instanceKlass.hpp
>> 
>>   No comment
>> 
>> src/hotspot/share/oops/valueKlass.cpp
>> 
>>   no comment
>> 
>> src/hotspot/share/oops/valueKlass.hpp
>> 
>>   line 109: allocate_instance_buffer() it would be helpful to have a comment here to explicitly
>>             state that the allocated instance memory has not been cleared, so initialization of
>>             its payload *must* follow before returning the oop to Java (there’s a comment in
>>             collectedHeap.hpp, but people have to dig in to find it)
> 
> Good spotting. This was left over code from attempting to improve the interpreter runtime call, I really should have given it a little more care than the initial benchmarking hack. Commented as:
> 
>   // allocates a stand alone value buffer in the Java heap
>   // DOES NOT have memory cleared, user MUST initialize payload before
>   // returning to Java (i.e.: value_copy)
> 
>> src/hotspot/share/gc/shared/barrierSetRuntime.cpp
>> 
>>   No comment
>> 
>> src/hotspot/share/gc/shared/barrierSetRuntime.hpp
>> 
>>   No comment
>> 
>> Regards,
>> 
>> Fred
> 
> 
>