RFR: Fixed two string dedup issues

[Zhengyu Gu]

On 10/10/2017 01:03 PM, Aleksey Shipilev wrote:
> On 10/10/2017 06:52 PM, Zhengyu Gu wrote:
>>
>>
>> On 10/10/2017 12:36 PM, Aleksey Shipilev wrote:
>>> On 10/10/2017 06:35 PM, Zhengyu Gu wrote:
>>>> On 10/10/2017 12:00 PM, Aleksey Shipilev wrote:
>>>>> On 10/10/2017 03:47 PM, Zhengyu Gu wrote:
>>>>>> Updated webrev: http://cr.openjdk.java.net/~zgu/shenandoah/strdedup_fix/webrev.01/index.html
>>>>> Can you explain a little more why do we need to filter for only Shenandoah safepoints? The string
>>>>> would only be queued, why does it matter if we are not in Shenandoah safepoint?
>>>> Okay, added comments.
>>>>
>>>
>>> I mean, can you explain here so I can understand what failure scenario you have in mind?
>>
>> String dedup protocol requires queuing during GC safepoints (where it suspends deplication thread)
>> and deduplicating strings outside GC safepoints.
> 
> That is what I don't understand. G1StringDedupQueue says:
> 
> // Pushing to the queue is thread safe (this relies on each thread using a unique worker
> // id), but only allowed during a safepoint. Popping from the queue is NOT thread safe
> // and can only be done by the deduplication thread outside a safepoint.

Yes, G1 does it at a safepoint with Dedup thread suspended, so each 
worker can safely push strings to queue.

Popping can only be done by Dedup thread, yes, but not dedeplication. 
Manipulating string dedup table always takes lock, it is MT-safe.


> 
> Notice "safepoint", not "GC safepoint". Where "GC safepoint" is coming from?
> 
>> G1 does all queuing during evacuation at GC safepoints, just as what we do with full GC.
> 
> Well, of course, it is STW. But it is non-sequitur why we need to check for *GC* safepoint for
> Shenandoah.
> 
>> Shenandoah evacuates objects inside and outside GC safepoints, so we need to know if it is a GC or
>> none GC safepoint, we only queue strings if it is a GC safepoint, otherwise, we disregard the
>> safepoint.
> 
> Look, you are explaining *what* happens in the code patch. I am asking *why* it happens. Notably,
> *why* the distinction between GC safepoints and non-GC safepoints matter? Why does it matter to
> check for Shenandoah-specific safepoint? Why can't we accept the push to dedup queue from the
> generic safepoint?

During concurrent evacuation, you may see periodic/clean safepoints, 
right? these safepoints are outside of our control, they do not suspend 
dedup thread, so it is unsafe to push.

safepoint test does not tell us if we are evacuating objects in parallel 
phase or concurrent phase, where we have to deal with differently. For 
shenandoah specific safepoints, we know dedup thread is suspended, can 
safely queue strings.


-Zhengyu








> 
> -Aleksey
>