RFR(M): Shenandoah string dedup support

[Zhengyu Gu]

On 08/24/2017 07:34 AM, Roman Kennke wrote:
> This here seems wrong:
> 
> + value1 = typeArrayOop(oopDesc::bs()->read_barrier(value1));
> + value2 = typeArrayOop(oopDesc::bs()->read_barrier(value2));
> + return (oopDesc::unsafe_equals(value1, value2) ||
> 
> the correct way to compare object equality is to use oopDesc::equals(). 
> Or am I missing something?
Okay, fixed


> 
> This does not look necessary:
> 
> - java_lang_String::set_value(java_string, existing_value);
> -
> + java_lang_String::set_value(java_string, 
> typeArrayOop(oopDesc::bs()->read_barrier(existing_value)));
> 
> 
> String::set_value() calls obj_field_put() which already does the RB on 
> the value.
> 
Fixed

> + // Shenandoah needs (special-1) rank of the lock, because write 
> barrier can evaculate objects while
> 
> s/evaculate/evacuate/
> 
> I assume you have thought hard about the locking stuff? No way to avoid it?
> 
I know this is ugly, but I don't see how to work around the scenario 
described in comments. Any suggestions?



> - } else if (UseG1GC || (UseShenandoahGC && ShenandoahSuspendibleWorkers)) {
> + } else if (UseG1GC || (UseShenandoahGC && 
> (ShenandoahSuspendibleWorkers || UseStringDeduplication))) {       SuspendibleThreadSet::synchronize();
> 
> Can you explain why we need to suspend/resume GC threads at safepoints 
> when using string dedup?

We need to suspend g1StringDedupThread, anyway to do so without 
suspending GC threads?

> 
> Roman
> 
> 
> Am 24.08.2017 um 13:07 schrieb Aleksey Shipilev:
>> On 08/23/2017 07:52 PM, Zhengyu Gu wrote:
>>>>    *) Do we need to do StringDedup cleanup on each concurrent mark? Right now all the cleanup actions
>>>> in final mark are done optionally (every 5-th cycle by default). Should we do the String table
>>>> cleanup there as well?
>>> Unless we want to mark the table (which I think it is a waste), otherwise, we will step on dead
>>> objects when update refs later.
>> Okay, then putting it under "System Purge" is misnomer:
>>
>> 1197   _phase_names[purge_str_dedup_table]           = "    String Dedup Table";
>>
>> Suggestion (different enum ID, and indenting):
>>
>> _phase_names[clean_dedup_table]           = "  String Dedup Table";

Fixed

>>
>>> Webrev updated:http://cr.openjdk.java.net/~zgu/shenandoah/strdedup/webrev.01/index.html
>> My bad, I missed two new files!
>>
>>   *) I think there is no reason to split declaration and definitions for methods in new closures
>> defined in shenandoahStringDedup.cpp.
>>
>>   *) Indenting:
>>
>>   152   ShenandoahUpdateRefsClosure      update_refs_closure;
>>
Fixed both.

Upated webrev: 
http://cr.openjdk.java.net/~zgu/shenandoah/strdedup/webrev.02/

Tests:

Reran all tests.

Thanks,

-Zhengyu


>> We need Roman to look through this as well.
>>
>> Thanks,
>> -Aleksey
>>
>