RFR: Shenandoah String Dedup refactoring

[Zhengyu Gu]

On 07/02/2018 04:44 PM, Roman Kennke wrote:
> 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..?

I assume you are talking about StringDedupQueue_lock and 
StringDedupTable_lock, right?

We adapted upstream's implementation, so yes, it is similar to SATB 
locks, we do need them too.

> 
> 
> 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.
> 

Kind of wonder where are we, in term of C++ version. ZGC already uses 
it. I asked Thomas Schatzl what's they takes on thread-local, he did not 
answer :-)


> 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.
> 

Thanks,

-Zhengyu

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