RFR (M) 8186042: Optimize OopMapCache lookup

[David Holmes]

Hi Coleen,

On 26/08/2017 12:05 AM, coleen.phillimore at oracle.com wrote:
> 
> This was reviewed on hotspot-runtime-dev but it was noticed that the 
> traversal of the interpreter's OopMapCache can be done outside of a 
> safepoint by the graal compiler and by stack walking.   This would make 
> reclamation of the "old" cached entries not safe.  I have disallowed use 
> of the interpreter OopMapCache in this change by any calls outside of 
> GC.   The OopMapCache is an optimization primarily (now exclusively) for 
> GC root walking.

That seems a very reasonable restriction. This looks good to me.

One nit: src/share/vm/oops/method.cpp

+  // and other uses generate an oopmap but do not save it in the cache.

and -> any?

Thanks,
David
-----


> open webrev at http://cr.openjdk.java.net/~coleenp/8186042.02/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8186042
> 
> Retested with jck tests with -Xlog:interpreter+oopmap=debug, hotspot 
> tier1 which includes the compiler AOT tests that failed, jvmti tests, 
> and stack walking tests in the jdk.
> 
> Thanks,
> Coleen
> 
> On 8/11/17 11:46 AM, coleen.phillimore at oracle.com wrote:
>> Summary: Use lock free access to oopMapCache
>> Contributed-by: frederic.parain at oracle.com, coleen.phillimore at oracle.com
>>
>> The OopMapCache::lookup() function took out a mutex to protect access 
>> between the GC threads that are running concurrently.  See bug for 
>> more info.   The function lookup() is run by multiple GC threads 
>> concurrently.  If there's a collision in the hashtable, this uses 
>> atomic cmpxchg to add the entry to a list to be cleaned up after the 
>> safepoint is over.   GC isn't doing lookup at that point.
>>
>> This change is contributed by Frederic Parain, with some cleanup and 
>> logging from me.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8186042.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8186042
>>
>> Tested with RBT equivalent of nightly on linux x64.  Also ran dacapo 
>> with -Xint -Xlog:interpreter+oopmap=debug to verify.  This change also 
>> removes -XX:+TraceOopMapGeneration (not -XX:+TraceNewOopMapGeneration 
>> however) in favor of new logging.  A linked CSR request is pending.
>>
>> Thanks,
>> Coleen
>>
>