ttyLocker and other locks

[Tom Rodriguez]

On Feb 21, 2011, at 3:33 PM, Vladimir Kozlov wrote:

> Tom,
> 
> I like your cache ciField changes and I wish we can apply it. One problem I found: ciInstanceKlass::compute_fields() returns 0 and don't cache static fields if there are no nonstatic fields.

Yes, it's mildly broken right now because I wasn't able to solve the shared ciField issue.

> 
> Can we throw out the compilation if static final fields state changed at the end of compilation as we do for jvmti? Yes, we have to keep the list of such fields to verify at the end of compilation.

I think if we can keep track of them to detect when they've changed then we can track them to make it work correctly.  I was thinking that the shared instances need to have some side state that is maintained on a per ciEnv basis so that for the lifetime of a compile we can return stable answers.  Once a shared instance reaches the fully_initialized state we can switch to using the stable shared state.  It's kind of complicated though.  Maybe the ciFields should just be tracked by ciEnv itself instead of maintaining the cache in the ciInstanceKlass.  ciFields are kind of an oddity right now since they aren't maintained in a central fashion the way that other ciObjects are.  I'm going to see what that would look like.

tom

> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> This is going to be a little long.  In the recent fix for 6354181, I added a lock of Compile_lock in the ciEnv when we call into the SystemDictionary for lookup.  It wasn't strictly required but it seemed like from reading the code that it would be advisable to hold it when querying the SystemDictionary.  Unfortunately this appears to cause some lock ordering problems when printing.  In particular we use a ttyLocker when we do PrintIdeal or PrintOptoAssembly and some of the printing logic looks up ciFields which may require calling into the SystemDictionary.  In particular MachNode::adr_type() may look up the field to figure out whether an oop field is compressed or not.  ciInstanceKlass caches the ciFields for instance fields for some lookups but not for statics.  I believe this is because the logic for creating shared ciInstanceKlasses wants to avoid caching the _constant_value for static fields when compiling during bootstrap.  I tried modifying the CI to always cache th
> e ciField so that ciFields were always created before we were holding the ttyLock but I couldn't resolve the shared ciField issue in a simple way.  The init_state of the shared instances may change during compilation which means that the ciField for a static final could become constant along the way and I don't see an easy way to maintain the fiction.  I actually think we're exposed to an issue here the shared instances can report changing information during a compile.  The partial rewrite I did to completely cache ciField is at http://cr.openjdk.java.net/~never/cik in case you want to see it.
>> Anyway, I'm out of good ideas to fix this other than removing the ttyLocker.  I actually think the ttyLocker itself is the problem.  It really only exists to encourage our output to be emitted more nicely than we get without it.  PrintOptoAssembly in particular produces fairly horrid interleaving when running with multiple compilers.  Narrowing the lock to just a single dump call doesn't help since the Compile_lock is acquired inside the dump call.  Vladimir suggested having a ttyUnlocker like construct that we could place before the lock of Compile_lock to release the lock if it was held and then reacquire it.  That's fairly ugly but I'm all out of pretty I think.
>> tom