RFR(M) 8203641: Refactor String Deduplication into shared

[Zhengyu Gu]

Thanks, Thomas.

-Zhengyu

On 06/14/2018 06:36 AM, Thomas Schatzl wrote:
> Hi Zhengyu,
> 
> On Tue, 2018-06-12 at 16:31 -0400, Zhengyu Gu wrote:
>> Hi Thomas,
>>
>> Thanks for the reviewing.
>>
>> On 06/12/2018 05:28 AM, Thomas Schatzl wrote:
>>>
>>> This would very likely decrease the amount of changes in the
>>> important change, the refactoring, significantly.
>>>
>>> Now everything is shown as "new" in diff tools, and we reviewers
>>> need to go through everything. It seems a bit of a stretch to call
>>> this "M" with 1800 lines of changed lines, both on the raw number
>>> of changes and the review complexity.
>>
>> I reshuffled some moved files, seems following updated webrev to
>> have better diff.
>>
>> http://cr.openjdk.java.net/~zgu/8203641/webrev.01/index.html
> 
> Thanks.
> 
>>>
>>> - I am not sure why g1StringDedup.hpp still contains a general
>>> description of the mechanism at the top; that should probably move
>>> to the shared files.
>>> Also it duplicates the "Candidate selection" paragraphs apparently.
>>> Please avoid comment duplication.
>>
>> Fixed.
> 
> Thanks. Could you rename "Candidate selection" to "G1 string
> deduplication candidate selection" in g1StringDedup.hpp? (Just rename
> the title of that paragraph)
> 
>>> I am not sure that keeping the interface related to string
>>> deduplication all static and then use instance variables behind the
>>> scene makes it easily readable.
>>>
>>> Making everything static has to me been an implementation choice
>>> because there has only been one user (G1) before.
>>
>> I kept this way to minimize changes in G1, especially, outside of
>> string deduplication code.
> 
> I see.
> 
>>>
>>> I will need to bring this up with others in the (Oracle-)team what
>>> they think about this. Probably it's okay to keep this, and this
>>> could be done at another time.
>>
>> Please let me know what is your decision, or file a RFE for future
>> cleanup.
> 
> In either case, it is best to do this as an extra CR.
> 
> [...]
>>> - in StringDedupThread::do_deduplication the template parameter
>>> changes from "S" (in the definition) to "STAT". Not sure why; also
>>> we do not tend to use all-caps type names.
>>
>> Fixed.
>>
>> Also, fixed the bug that caused crashes you mentioned.
>>
>> Ran runtime_gc tests with -XX:+UseStringDeduplication on Linux x64
>> (fastdebug | release).
>>
> 
> Retesting looks good from our side too.
> 
> The change seems good too. I do not need to see a new webrev for above
> mentioned comment change.
> 
> Thanks,
>    Thomas
>