RFR(M) 8203641: Refactor String Deduplication into shared

[Zhengyu Gu]

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

> 
> Please, in the future use two CRs or provide two webrevs in one review
> in such a case. This would make the reviewing a lot less work for
> reviewers and turnaround a lot faster.

Will do

> 
> Some initial comments anyway:
> 
> - the change may not apply cleanly any more, sorry for the delay. At
> least it complains about "src/hotspot/share/gc/g1/g1StringDedup.[hc]pp
> is not empty after patch; not deleting".
> Maybe it is a limitation of the "patch" tool that incorrectly prints
> this. It builds though.
> Probably you first moved the files and then recreated them?

Yes, it is due to reuse the same filenames.

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

> 
> - the comment on G1StringDedupQueue does not describe the queue at all
> but seems to be some random implementation detail.
> Maybe put all the G1 specific considerations in g1StringDedup.hpp - and
> only these?
> 
> (I saw that stringDedup.hpp refers to "gc specific" files, which is
> fine)
You mean StringDedupQueue, right?

I moved implementation related comments into g1StringDedup queue, while 
kept general mechanism in stringDedupQueue, ok?

> 
> - generally, if a definition of a method in a base class is commented,
> describing its contract, it is not necessary to duplicate it in the
> overriding methods.
> That just makes it prone to getting out of date.
> 
Fixed

> - maybe instead of a "queue_" prefix for the protected
> G1StringDedupQueue methods, use "_impl" as elsewhere.

Fixed.

> 
> 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 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 stringDedupStat.cpp remove commented out renmants of generational
> statistics (line 121+,152+)

Done.

> 
> - some copyright years need to be updated I guess.
>

Done.

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

Thanks,

-Zhengyu

> 
> I will run it through our testing infra with/without string dedup and
> then look through it some more.
> 
> Thanks,
>    Thomas
> 
>