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

[Jiangli Zhou]

Hi John,

Thanks for the comments! I'll incorporate them.

Thanks,
Jiangli

On 02/08/2012 05:04 PM, John Rose wrote:
> 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/4c7ee412/attachment.html