RFR(M) 8203641: Refactor String Deduplication into shared

[Thomas Schatzl]

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