RFR (XXL) [7u60]: nmethod backports (12 backports)

[Vladimir Kozlov]

Thomas,

Just few notes.

Make sure when you do backport you should use the main bug id in changeset. For example, for 7145569 backport it should 
be the same 7145569 and not 8025423.

In your table second change lists incorrect bug id for jdk8 (the same as in first line). Should be 8013132 instead of 
8011343. The same problem with last change: should be 8020123 instead of 8027756. The actual links are correct.


For 7145569 (in 8 and in 7u60), in sweeper.hpp new method was added which is not used:

+  static bool is_sweeping(nmethod* which) { return _current == which; }

In 801313 (both version) typo in globals.hpp - double "print":

+   diagnostic(bool, VerifySilently, false,                                   \
+           "Don't print print the verification progress")                    \

Both above problems are not critical and we can leave them.


I mostly worry about scanning roots from nmethods (codecache) which may lead to incorrect nmethods unloading.
What is ad-hoc testing you listed? And I don't see kitchensink test in your list.

Regards,
Vladimir

On 1/18/14 3:09 PM, Thomas Schatzl wrote:
>
> Hi all,
>
>    please review the backport of the nmethod optimization changes for G1
> (7145569) and all related issues.
>
> This request in total contains 12 changes, five which enable easy
> backport of the above mentioned main change, and six that fix issues
> with the original change.
>
> These are
>
> Add new flag for verifying the heap during startup (8029486)
> Add a flag to turn off the output of the verbose verification code (8029487)
> G1: Add remembered set size information to output of G1PrintRegionLivenessInfo (8028187)
> G1: G1SummarizeRSetStats output on Linux needs improvement (8028186)
> G1: Verification after a full GC is incorrectly placed. (8029596)
> G1: optimize nmethods scanning (8025423)
> G1: G1CollectedHeap::mark_strong_code_roots() needs to handle ParallelGCThreads=0 (8028193)
> G1: improve remembered set summary information by providing per region type information (8028188)
> G1: assert "assert(thread < _num_vtimes) failed: just checking" fails when G1ConcRefinementThreads > ParallelGCThreads (8028189)
> -XX:+G1SummarizeRSetStats can result in wrong exit code and crash (8028190)
> assert(!hr->isHumongous()) failed: code root in humongous region? (8028191)
> Test gc/g1/TestPrintRegionRememberedSetInfo.java fails with "test result: Error. No action after @build" (8029488)
>
> To track these backport CRs, the original CRs and their webrevs,
> I created an overview page for all changes at
> http://cr.openjdk.java.net/~tschatzl/nmethod-backport/ , listing the jdk
> 7u60 bug IDs and links to webrevs, and the corresponding original jdk 8
> bug IDs and webrevs.
>
> Except for the main change (JDK-8025423, JDK-7145569) there were no
> particular changes necessary (possibly adding a missing method from
> another change verbatim), so I think reviews should ooncentrate on that.
> Of course, feel free to comment on the other backports.
>
> Some notes about the backport:
>
> - jdk7 has the old perm gen, so some changes simply did not apply at
> all. E.g. there is no YoungRefCounterClosure to move, no
> VerifyKlassClosure etc.
>
> - there is one important change in how
> G1CollectedHeap::g1_process_strong_roots() and
> SharedHeap::process_strong_roots() are called.
>
> hs24(jdk7) and hs25(jdk8) handle code cache roots differently. In
> SharedHeap::process_strong_roots() there is the option that the caller
> sets SO_CodeCache to scan code cache roots. The problem is that
> even if SO_CodeCache is _not_ set, jdk7 iterates over some of the code
> roots that have references into the heap anyway. Additionally this is
> also dependent on whether we are collecting the perm gen or not.
>
> This is different to hs25 where SharedHeap::process_strong_roots() only
> ever iterates code roots when SO_CodeCache is set.
>
> Now, with the nmethod optimization, we do not want
> SharedHeap::process_strong_roots() to do any code root iteration like in
> JDK8.
>
> This unfortunately changed as part of the (huge) initial perm gen removal
> patch.
>
> Instead of trying to reorganizing this code, and looking through all
> invocations to SharedHeap::process_strong_roots() to set the correct
> flags (plus verification), I introduced a new boolean to
> SharedHeap::process_strong_roots() called "manages_code_roots" that
> indicates whether the caller handles code roots by itself in the case
> when we are not explicitly passing SO_CodeCache.
>
> This flag is only set to true (with a default value of false) in case of
> G1 young collection, so minimizing code changes.
>
> As far as I can see, the alternative is to implement something like in
> JDK8 here: add a is_scavengable flag and look through all invocations to
> process_strong_roots() to pass the SO_CodeCache flag.
>
> As mentioned, I tried to minimize changes here, I am open to suggestions
> here to improve that part though.
>
> - in (Un-)RegisterNMethodOopClosure::do_oop_work() the code needs to ignore
> references into the permanent generation. In JDK8 these closures do not
> get references that are embedded into the code into the "perm gen" any
> more. They are simply not oops.
>
> - added GrowableArray::find_from_end() and GrowableArray::max_length(),
> also from that initial perm gen removal change.
>
> - for the backport in JDK-8028191 I had to add a "deoptimizeAll"
> Whitebox method that calls a compiler method to deoptimize all code. This
> was part of a larger compiler change that has not been backported.
>
> Improvement:
> Reduces young gc pause time in some FMW applications by a significant
> amount, particularly for code that has a significant amount of compiled
> code, e.g. from ~160ms to ~30ms for young-only GCs in CRM Fuse.
>
> Testing:
> jtreg, jprt, FMW applications, dacapo, specjbb05, ad-hoc (adhoc
> runs with default collector and G1, fastdebug + product) testing
>
> Comparing to a vanilla 7u60 adhoc testing run, there were three
> occurrences of https://bugs.openjdk.java.net/browse/JDK-8030803 that
> has been fixed in jdk8 but apparently is not scheduled for 7u60. It
> is completely unrelated to the changes from what I can see, and a
> known issue in 7u60.
>
> Thanks a lot,
>    Thomas
>
>
>
>
>