RFR: 8212682: Avoid holding Compile_lock when blocking for GC in ObjArrayKlass::allocate_objArray_klass()

[coleen.phillimore at oracle.com]

I wonder if the Compile_lock is added for this in ci/ciEnv.cpp (and it's 
copy in jvmtiEnv.cpp):

ciKlass* ciEnv::get_klass_by_name_impl(ciKlass* accessing_klass,
...
     MutexLocker ml(Compile_lock);
     Klass* kls;
     if (!require_local) {
       kls = 
SystemDictionary::find_constrained_instance_or_array_klass(sym, loader,
KILL_COMPILE_ON_FATAL_(fail_type));
     } else {
       kls = SystemDictionary::find_instance_or_array_klass(sym, loader, 
domain,
KILL_COMPILE_ON_FATAL_(fail_type));
     }
     found_klass = kls;
   }

which eventually calls in SystemDictionary::

     if (k != NULL) {
       k = k->array_klass_or_null(fd.dimension());
     }


And maybe the Compile_lock keeps the Klass::_higher_dimension have a 
consistent value.  Maybe the code in ciEnv.cpp and jvmciEnv.cpp should 
take MultiArray_lock instead.  The problem is that Compile_lock protects 
too many things.

I don't know if this is safe to remove unfortunately.

Coleen


On 10/23/18 10:59 AM, Erik Österlund wrote:
> Hi,
>
> We occasionally blockingly wait for GC in allocations in 
> ObjArrayKlass::allocate_objArray_klass(), while holding the Compile_lock.
> This is problematic for concurrent class unloading that needs to hold 
> that lock in the unloading path. This introduces a potential deadlock 
> situation.
>
> After staring enough at this code together with help from Coleen, I 
> have convinced myself that the Compile_lock is in fact not needed in 
> this path, and there is nothing to my knowledge it actually protects 
> here. The vague comment next to the lock suggests its purpose is to 
> protect vtable stubs. But it doesn't. There is a VtableStubs_lock for 
> that, and I find no traces if the Compile_lock having anything to do 
> with that.
>
> Coleen helped me trying to trace down where this locking code came 
> from. It takes us back to before anyone was around, and does not seem 
> to have changed in the past 2 decades, and its origins are a bit 
> unclear. The theory is that perhaps vtable stubs used to be protected 
> by the Compile_lock in ancient times, and the locking code is still 
> around because nobody dared to touch it.
>
> Since both code analysis and mach5 suggest there is no reason for this 
> lock to be held here, I propose to remove it with this patch.
>
> If anyone that has been around for more than 2 decades in HotSpot 
> happens to know the reason why this locking was introduced, any 
> commentary around that would be very appreciated.
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8212682/webrev.00/
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8212682
>
> Thanks,
> /Erik