[9] RFR (M): VM should constant fold Unsafe.get*() loads from final fields

[John Rose]

On Jun 17, 2015, at 9:38 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> 
> http://cr.openjdk.java.net/~vlivanov/8078629/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8078629
> 
> Direct(getfield/getstatic) read operations are faster than unsafe reads from constant Java fields, since VM doesn't constant fold unsafe loads. Though VM tries hard to recover field metadata from its offset, it doesn't optimize unsafe ones even if it has all necessary info in its hands.
> 
> The fix is to align the behavior and share relevant code between C2 parser and intrinsic expansion logic.
> 
> For testing purposes, I extended whitebox API to check whether a value is a compile-time constant. The whitebox test enumerates all combinations of a field and ensures that the behavior is consistent between bytecode and unsafe reads.
> 
> Testing: focused whitebox tests, hotspot/test/compiler, jdk/test/java/lang/invoke, octane (for performance measurements)

WB.isCompileConstant is a nice little thing.  We should consider using it in java.lang.invoke
to gate aggressive object-folding optimizations.  That's one reason to consider putting it
somewhere more central that WB.  I can't propose a good place yet.  (Unsafe is not quite right.)

The gating logic in library_call includes this extra term:  && alias_type->field()->is_constant()
Why not just drop it and let make_constant do the test (which it does)?

You have some lines with "/*require_const=*/" in two places; that can't be right.
This is the result of functions with too many misc. arguments to keep track of.

I don't have the code under my fingers, so I'm just guessing, but here are more suggestions:

I wish the is_autobox_cache condition could be more localized.  Could we omit the boolean
flag (almost always false), and where it is true, post-process the node?  Would that make
the code simpler?

This leads me to notice that make_constant is not related strongly to GraphKit; it is really
a call to the Type and CI modules to look for a singleton type, ending with either a NULL
or a call to GraphKit::makecon.  So you might consider changing Node* GK::make_constant
to const Type* Type::make_constant.

Now to pick at the argument salad we have in push_constant:  The effect of is_autobox_cache
could be transferred to a method Type[Ary]::cast_to_autobox_cache(true).
And the effect of stable_type on make_constant(ciCon,bool,bool,Type*),
could also be factored out, as post-processing step contype=contype->Type::join(stabletype).

— John