RFR: 8149837: String.intern creates more work than necessary for G1

[Stefan Karlsson]

Hi Derek,

This version looks good to me.

Thanks,
StefanK

On 16/02/16 19:42, Derek White wrote:
> 2nd webrev puts the calls to ensure_string_alive() back to their 
> original location (to avoid locking issues mentioned by Stefan).
>
> RFE: JDK-8149837 <https://bugs.openjdk.java.net/browse/JDK-8149837>
> Webrev: http://cr.openjdk.java.net/~drwhite/8149837/webrev.02/
>
> Thanks for looking!
>
>  - Derek
>
> On 2/15/16 4:57 PM, Derek White wrote:
>> Hi Stefan,
>>
>> On 2/15/16 3:55 PM, Stefan Karlsson wrote:
>>> Hi Derek,
>>>
>>> I don't think this change will work, unless something changed in 
>>> this area recently. We used to have the code the way your patch is 
>>> written, but unfortunately we end up with a lock reordering problem 
>>> with the StringTable_lock and one of the locks taken when executing 
>>> the G1SATBCardTableModRefBS::enqueue. Unfortunately, there's no 
>>> comment in the code that shows this intricate problem.
>>>
>>> This is the problematic code:
>>>   241
>>>   242   // Grab the StringTable_lock before getting the_table() because it could
>>>   243   // change at safepoint.
>>>   244   oop added_or_found;
>>>   245   {
>>>   246     MutexLocker ml(StringTable_lock, THREAD);
>>>   247     // Otherwise, add to symbol to table
>>>   248     added_or_found = the_table()->basic_add(index, string, name, len,
>>>   249                                   hashValue, CHECK_NULL);
>>>   250   }
>>>   251
>>>   252   ensure_string_alive(added_or_found);
>>>   253
>>>   254   return added_or_found;
>>>   255 }
>>> We hold StringTable_lock, while calling this chain: basic_add -> 
>>> lookup_in_main_table -> ensure_string_alive -> 
>>> G1SATBCardTableModRefBS::enqueue.
>>>
>>> I'm not sure, but I think this was the code that took the lock:
>>> void PtrQueueSet::enqueue_complete_buffer(void** buf, size_t index) {
>>>   MutexLockerEx x(_cbl_mon, Mutex::_no_safepoint_check_flag);
>>>
>>> and if that's the case you might be able to provoke the problem by 
>>> setting the G1SATBBufferEnqueueingThresholdPercent flag.
>>>
>>> Have you run this patch with fastdebug on a larger set of benchmarks?
>> OK, I was wondering if there was something deeper that I wasn't 
>> seeing. I haven't tried with a larger benchmark case.
>>>
>>> I think we should hold this patch until we have figured out if this 
>>> is still a problem or not.
>>
>> OK, I can also go back to my plan A, which left the calls to 
>> ensure_string_alive() in the same place, but made the calls 
>> conditional on whether or not the string returned from the "intern" 
>> was the same as the string passed in.
>>
>> Thanks for the quick response!
>>>
>>> Thanks,
>>> StefanK
>>>
>>> On 2016-02-15 20:52, Derek White wrote:
>>>> This small change only does a STAB read barrier if we really read a 
>>>> string from the string intern table.
>>>>
>>>> This is a small optimization, and the old code may contribute to 
>>>> the malloc failures in JDK-8134992 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8134992> 
>>>> "vm/gc/compact/Compact_InternedStrings_Strings failed due to a 
>>>> malloc() failure"
>>>>
>>>> RFE: JDK-8149837 <https://bugs.openjdk.java.net/browse/JDK-8149837>
>>>> Webrev: http://cr.openjdk.java.net/~drwhite/8149837/webrev.01/
>>>>
>>>> Tested: jprt
>>>>
>>>> Thanks!
>>>>  - Derek
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160216/26dc7be9/attachment.htm>