RFR (M): 8217778: StringTable cleanup miscalculates amount of dead objects

[Thomas Schatzl]

Hi Kim and Coleen,

On Mon, 2019-01-28 at 22:24 -0500, Kim Barrett wrote:
> On Mon, 2019-01-28 at 12:43 +0100, Thomas Schatzl wrote:
> > Hi all,
> > 
> >  can I have reviews for this change that tries to fix the issue
> > with the string table reporting too low "dead" numbers to the
> > ServiceThread notification mechanism in the review for JDK-8213229,
> > potentially causing C heap memory being kept live for longer than
> > necessary?
> > 
> > This change implements one option for fixing this, in particular
> > that the WeakProcessor calls OopStorage::oops_do() instead of
> > ::weak_oops_do, counting the number of NULL entries manually.
> 
> I think this is the right approach to take.
> 

Good.

> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8217778
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8217778/webrev/
> > Testing:
> > hs-tier1-5
> > 
> > Thanks,
> >  Thomas
> 
> Looks good.
> 
> A couple minor cleanup suggestions.  I don't need another webrev if
> you decide to do these.
> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/shared/weakProcessor.cpp
>   41       CountingIsAliveClosure<BoolObjectClosure> cl(is_alive);
> 
> Why are we counting here?  We don't do anything with the resulting
> count.  (I missed this when reviewing JDK-8213229.)

Fixed.

> 
> Similarly here, when !is_stringtable(phase):
>   44       CountingSkippedIsAliveClosure<BoolObjectClosure,
> OopClosure> cl(is_alive, keep_alive);
> 
> There could instead be a separate is_stringtable(phase) clause which
> had the calls to StringTable::(reset|finish)_dead_counter() and the
> counting closure stuff all close together and associated only with
> the stringtable storage iteration.

Done.

> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/shared/weakProcessor.inline.hpp 
>   74     if (obj == NULL) {
>   75       _num_skipped++;
>   76       return;
>   77     }
>   78     if (_counting_is_alive.do_object_b(obj)) {
>   79       _keep_alive->do_oop(p);
>   80     } else {
>   81       *p = NULL;
>   82     }
> 
> This would be simpler if the line 76 "return" were removed and the
> line 78 "if" were an "else if", having a single if-ladder.

Done.

> 
> -------------------------------------------------------------------
> -----------

(from the other email)


> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/shared/weakProcessor.cpp
>   44       CountingSkippedIsAliveClosure<BoolObjectClosure,
> OopClosure> cl(is_alive, keep_alive);
> 
> A different approach is possible in this serial case.  We could wrap
> the keep_alive closure for counting, as the number we want to pass to
> inc_dead_counter is also available here as
> 
>   storage->allocation_count() - <number of keep_alive calls>
> 
> -------------------------------------------------------------------
>

I did not implement that. It seems to look too different from the
parallel case with no difference in actual work done. And we need the
CountingSkipped.. closure anyway for the parallel case.

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8217778/webrev.0_to_1 (diff)
http://cr.openjdk.java.net/~tschatzl/8217778/webrev.1 (full)

Thanks,
  Thomas