RFR(M) 8203641: Refactor String Deduplication into shared
zgu at redhat.com
Tue Jun 12 20:31:44 UTC 2018
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
> 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?
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.
> - 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
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.
> - 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 kept this way to minimize changes in G1, especially, outside of string
> 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+)
> - 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.
Also, fixed the bug that caused crashes you mentioned.
Ran runtime_gc tests with -XX:+UseStringDeduplication on Linux x64
(fastdebug | release).
> I will run it through our testing infra with/without string dedup and
> then look through it some more.
More information about the hotspot-dev