Please review fix for 7120481 storeStore barrier in constructor with final field
jiangli.zhou at oracle.com
Wed Feb 8 17:08:04 PST 2012
Thanks for the comments! I'll incorporate them.
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:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev