RFR (M) 8186042: Optimize OopMapCache lookup

[coleen.phillimore at oracle.com]

On 8/15/17 12:55 PM, serguei.spitsyn at oracle.com wrote:
> Hi Coleen,
>
> It looks good in general.

Thank you Serguei for the code review.

> One comment though.
>
> http://cr.openjdk.java.net/~coleenp/8186042.01/webrev/src/share/vm/interpreter/oopMapCache.cpp.frames.html
>
>   516   // Entry is not in hashtable.
> 517 // Compute entry
> 518
> 519 OopMapCacheEntry* tmp = NEW_C_HEAP_OBJ(OopMapCacheEntry, mtClass);
> 520 tmp->initialize();
> 521 tmp->fill(method, bci);
> 522 entry_for->resource_copy(tmp);
>   523
>   524   if (method->should_not_be_cached()) {
>   525     // It is either not safe or not a good idea to cache this Method*
>   526     // at this time. We give the caller of lookup() a copy of the
>   527     // interesting info via parameter entry_for, but we don't add it to
>   528     // the cache. See the gory details in Method*.cpp.
> 529 FREE_C_HEAP_OBJ(tmp);
>   530     return;
>   531   }
>
The OopMapCacheEntry we allocate at 519 is used to fill in the entry_for 
passed in at line 522, so it can't be moved.  The fill() method does the 
abstract interpretation, filling in tmp and that's used to fill in 
entry_for.   entry_for is an InterpreterOopMap, which is derived from 
OopMapCacheEntry.  It's sort of odd code, ie not straightforward.

Thanks,
Coleen


>  Would it better to move the fragment 524-531 above the line 516?
>  Then the line (529 FREE_C_HEAP_OBJ(tmp);) could be removed.
>
>
> Thanks,
> Serguei
>
>
> On 8/11/17 08:46, 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
>>
>