RFR: Shenandoah String Dedup refactoring

[Roman Kennke]

Is it really necessary to use global locks? Is there no instance that
can 'carry' the lock? I guess it's not a Shenandoah specific lock
though, right? IOW, similar issue as with SATB locks..?


Thread-local is not a language feature, yet. Coming with c++11. I
believe it's ok to put in ShenandoahThreadLocalData as long as we don't
need to extend its size. We should have some room left because of ZGC
:-) Should we ever switch to C++11 we'd use real thread_local though, I
guess.

I trust you with the actual dedup code, there is not much I can review
there that you don't know better.

The rest of patch looks ok to me.

Thank you for doing this!
Roman


> Hi,
> 
> Can I get a review?
> 
> Updated to current shenandoah/jdk head.
> 
> Webrev:
> http://cr.openjdk.java.net/~zgu/shenandoah/shared_stringdedup/webrev.02/
> 
> 
> Reran: tier3_gc_shenandoah (fastdebug + release), all with
> -XX:+UseStringDeduplication.
> 
> Thanks,
> 
> -Zhengyu
> 
> On 06/29/2018 07:23 AM, Zhengyu Gu wrote:
>> Hi,
>>
>> This is updated Shenandoah string deduplication based on official
>> upstream shared implementation.
>>
>> Based on Roman's comment, I moved worker id into
>> ShenandoahThreadLocalData, instead of using language feature.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~zgu/shenandoah/shared_stringdedup/webrev.01/
>>
>> Test:
>>
>>    tier3_gc_shenandoah, all tests with -XX:+UseStringDeduplication
>>    (release + fastdebug)
>>
>>    specjvm with -XX:+UseStringDeduplication
>>
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>> On 06/02/2018 04:39 AM, Roman Kennke wrote:
>>>
>>> Is that a thread-local variable ?:
>>>
>>> +  static __thread uint _worker_id;
>>>
>>> Is there no better way to achieve the same? E.g. we don't want to put it
>>> into ShenandoahThreadLocalData or such? The worker_id seems small enough
>>> to actually fit into a byte. Maybe we have a byte-field left in ShTLD
>>> without requiring to blow up its word-size?
>>>
>>> Other than that, looks good.
>>>
>>> Thank you!!
>>> Roman
>>>
>>>> Shenandoah only webrev:
>>>> http://cr.openjdk.java.net/~zgu/shenandoah/dedup_shared_refactor/sh-only/webrev.00/
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>> On 05/29/2018 03:54 PM, Zhengyu Gu wrote:
>>>>> Hi,
>>>>>
>>>>> This refactoring is based on upstream JDK-8203641, which is still
>>>>> under review
>>>>> (http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-May/032565.html).
>>>>>
>>>>>
>>>>>
>>>>> We initially borrowed G1's string deduplication implementation, but we
>>>>> diverged at one point, due to we may need to enqueue candidates inside
>>>>> write barrier. It is not the case anymore.
>>>>>
>>>>> Once JDK-8203641 upstreamed, we can share dedup table and thread
>>>>> implementation, only need to plugin in our queue implementation.
>>>>>
>>>>> It looks like StringDedupTable is another candidate to be replaced by
>>>>> ConcurrentHashTable, we should be able to benefit from it too.
>>>>>
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~zgu/shenandoah/dedup_shared_refactor/webrev.00/
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Test:
>>>>>     hotspot_gc_shenandoah (fastdebug and release)
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Zhengyu
>>>>>
>>>
>>>