RFR(M) 8203641: Refactor String Deduplication into shared

[Thomas Schatzl]

Hi,

  testing showed that some error has been introduced. It can be
reliably reproduced by running the test

  gc/stress/TestGCBasherWithG1.java

with -XX:+UseStringDeduplication set.

An example command line could be:

jtreg -jdk:<path-to-jdk> -vmoption:-XX:+UseStringDeduplication
gc/stress/TestGCCBasherWithG1.java

There were some other failed tests, but they all looked very similar.
The crash does not occur without these changes.

Please fix this issue. :)

It would be nice if you ran through all jtreg tests with
-XX:+UseStringDeduplication to check any other issues. There are not
many tests that explicitly use -XX:+UseStringDeduplication, but of
course a run of all tests with string dedup enabled should give a good
coverage.

Thanks,
  Thomas

On Tue, 2018-06-12 at 11:28 +0200, Thomas Schatzl wrote:
> 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
> 
>