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

[David Simms]

Thanks again !

On 22/11/19 3:16 PM, Frederic Parain wrote:
> 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
>>
>>