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

[Derek White]

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/6c594acb/attachment.htm>