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

[Derek White]

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/20160215/49706b2f/attachment.htm>