RFR(M) 8203641: Refactor String Deduplication into shared

[Thomas Schatzl]

Hi,

On Mon, 2018-05-28 at 17:11 -0400, Zhengyu Gu wrote:
> Hi,
> 
> Please review this refactoring of G1 string deduplication into
> shared directory, so that other GCs (such as Shenandoah) can
> advantage of existing infrastructure and plugin their own
> implementation.
> 
> This refactoring preserves G1's String Deduplication infrastructure 
> (please see the comments in stringDedup.hpp for details), so that
> there is no change to G1 outside of string deduplication code.

  would it be possible to provide separate diffs for moving the files
and applying the refactoring? (And do the minimal changes to make it
compile).

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.

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.

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?

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

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

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

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

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

- in stringDedupStat.cpp remove commented out renmants of generational
statistics (line 121+,152+)

- some copyright years need to be updated I guess.

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

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

Thanks,
  Thomas