RFR(M): Shenandoah string dedup support

[Roman Kennke]

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?

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.

+ // 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?

- } 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?

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";
>
>> 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;
>
> We need Roman to look through this as well.
>
> Thanks,
> -Aleksey
>