RFR: 8240556: Abort concurrent mark after effective eager reclamation of humongous objects

[Thomas Schatzl]

Hi Albert,

   thanks for your review.

On 18.09.20 12:22, Albert Mingkun Yang wrote:
 > On Tue, 15 Sep 2020 12:40:46 GMT, Thomas Schatzl 
<tschatzl at openjdk.org> wrote:
 >
 >> Hi all,
 >>
 >>    please review this change that implements concurrent mark abort 
as proposed by Liang Mao from Alibaba.
 >>
 >> Basically, if at the end of the concurrent start pause we notice that
 >> we went below the IHOP threshold (and it has been
 >> a concurrent start pause caused by humongous allocation), instead of
 >> scheduling a mark operation, we schedule a
 >> "concurrent undo" operation that undoes the changes.  Regarding the
 >> removal of the
 >> 
test/hotspot/jtreg/gc/stress/jfr/TestStressBigAllocationGCEventsWithG1.java 
test: it only ever
 >> accidentally worked in G1. G1 never sent the AllocationRequiringGC
 >> events for GCs caused by going over the IHOP threshold for humongous
 >> allocations that the test actually expects.  That test previously
 >> only worked because G1 could not reclaim the humongous objects fast
 >> enough so crossing the IHOP threshold causes a full concurrent mark.
 >> Allocations during that concurrent mark do not cause a GC that can
 >> reclaim these objects, so ultimately some young GC that sends the
 >> AllocationRequiringGC event will be sent.  With concurrent undo this
 >> is not guaranteed any more, i.e. only in environments where
 >> concurrent undo is slow (and we'll improve it soon) this test works.
 >> The test is too timing dependent, and checking for the wrong thing in
 >> this case, so removing it.  Testing: tbd.
 >
 > src/hotspot/share/gc/g1/g1ConcurrentMarkThread.hpp line 112:
 >
 >> 110:   bool idle() const;
 >> 111:   bool started() const;
 >> 112:   bool in_progress() const;
 >
 > What's the motivation of moving them out of the header? Since they are
 > quite small, placing them in the header makes
 > inlining possible and desirable.

This way the implementation does not distract from reading the .hpp file 
as they've become larger.

There is no gain in inlining them imho as the callers are not 
performance sensitive (as in: this is called once now and then), so I 
did not add a .inline.hpp file.

 >
 > src/hotspot/share/gc/g1/g1ConcurrentMarkThread.hpp line 43:
 >
 >> 41:   G1ConcurrentMark* _cm;
 >> 42:
 >> 43:   enum ServiceState : uint {
 >
 > Why `: uint`? The same question for `enum Command` below as well.
 >

C++14 (actually 11 already) finally allows specification of a type, and 
I prefer the certainty of having a particular type over the uncertainty 
of some random type chosen by the compiler. That's just personal style.

There is also the possibility to use class enums, but since these are 
private enums I did not see the need to use it as they do not use the 
public class namespace anyway.

I saw the changes here as an opportunity to change all enums that I was 
changing anyway to get a proper type.

Thanks,
   Thomas