Please review fix for 7120481 storeStore barrier in constructor with final field

[John Rose]

That output code looks very nice.  And it looks like a reasonable approach to fixing the ordering problems.  But the implementation code needs some adjustments.

I haven't reviewed it all, but I have a couple of comments:

ciMethod.hpp : Please do not put state on the CI types.  They are read-only mirrors into the JVM metadata.  If you need compiler-local state, use the data structures in src/share/vm/c1.  I would look at IRScope as a candidate (compare _number_of_locks).

rewriter.hpp : Please do not make TRAPS an optional argument.  That subverts the static checking that the TRAPs macro is designed for.
Also, do not use TRAPS as a shorthand for passing a thread argument.   It means something else.
I suggest that you just pass what you need (the constantPoolHandle, not the thread and the instanceKlassHandle).
The logic for checking for final fields is wrong; it looks up the name/type of the field in klass, whereas it should respect the klass named in the constant pool.

Adding a new bytecode (_return_with_membar) is a relatively high-cost change, and requires many touches in many places in the source base.  I suspect you have not found them all yet, since a grep for _return_register_finalizer shows files not on your webrev.

But if you are going to use the new-interpreter-instruction tactic, I suggest you merge your new logic into _return_register_finalizer, using that instruction as a general "slow path" form of "_return".  (You could rename it as _return_special; check with Tom who wrote it.)  There will be no difference in performance.  Adding a new interpreter bytecode requires a strong performance justification, and you only need one "special return".

-- John

On Feb 8, 2012, at 3:35 PM, Jiangli Zhou wrote:

> Please review the following webrevs for 7120481 storeStore barrier in constructor with final field:
> 
> http://javaweb.sfbay.sun.com/~jianzhou/webrev.storestore/
> http://javaweb.sfbay.sun.com/~jianzhou/webrev.storestore.closed/

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120208/b37f4acd/attachment.html