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

[Derek White]

Thanks Stefan!

On 2/16/16 1:49 PM, Stefan Karlsson wrote:
> 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/952b3aca/attachment.htm>