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

[David Simms]

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