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

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Wed Jun 24 12:59:13 UTC 2015

John, Paul, thanks for review!

Updated webrev: http://cr.openjdk.java.net/~vlivanov/8078629/webrev.01/

I spotted a bug when field and accessor types mismatch, but the JIT still constant-folds the load. The fix made expected result detection even more complex, so I decided to get rid of it & WhiteBox hooks altogether. The test exercises different code paths and compares returned values now.

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.) Actually, there's already j.l.i.MethodHandleImpl.isCompileConstant. Probably, compiler-specific interface is the right place for such things. But, as I wrote before, I decided to avoid WB hooks.

The gating logic in librarycall includes this extra term: && aliastype->field()->isconstant() Why not just drop it and let makeconstant do the test (which it does)? I wanted to stress that make_constant depends on whether the field is constant or not. I failed to come up with a better method name (try_make_constant? make_constant_attempt), so I decided to keep the extra condition.

You have some lines with "/requireconst=/" 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: Thanks! I tried to address all your suggestions in updated version.

Best regards, Vladimir Ivanov

I wish the isautoboxcache 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 makeconstant 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::makeconstant to const Type* Type::makeconstant. Now to pick at the argument salad we have in pushconstant: The effect of isautoboxcache could be transferred to a method Type[Ary]::casttoautoboxcache(true). And the effect of stabletype on makeconstant(ciCon,bool,bool,Type*), could also be factored out, as post-processing step contype=contype->Type::join(stabletype).

More information about the hotspot-compiler-dev mailing list