RFR(M) 8203641: Refactor String Deduplication into shared
thomas.schatzl at oracle.com
Thu Jun 14 10:36:18 UTC 2018
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.
> > - 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.
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 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
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.
> 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.
More information about the hotspot-dev