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

[Kim Barrett]

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.

> There is another, let weak_oops_do() return the number of NULL
> entries as function return value. However the return value of the
> used iterate_safepoint() is already in use in some I'd consider
> interesting test case. I did not want to add another out-parameter
> either.

That short-circuiting behavior and result is to support some places in
JVMTI that have not (yet) been modified to make use of the feature.
There might be other places that could benefit.

> Since the WeakProcessor has been the main user of the weak_oops_do()
> method, I would suggest to clean this up preferably in an extra CR:
> there is still one use of the OopStorage::weak_oops_do() method that
> could be easily removed. If you think we should not remove it, I
> strongly suggest to rename the weak_oops_do() methods to better
> indicate what they actually do instead of giving a very strong
> (wrong) suggestion where/when they should be used.

Where is the remaining weak_oops_do? I don't have any objection to it
being removed, as it exists only for convenience of callers. If there
aren't any callers...

As I said privately, OopStorage::weak_oops_do is just following a
common idiom used elsewhere, where it's a shorthand for an oops_do
with a closure that does
  if-nonnull-then-if-isalive-then-keepalive-else-clear
There is nothing wrong with applying oops_do to an OopStorage
containing weak oops, and in fact there are various places that do
exactly that.  Or said differently, it is not necessary to use
weak_oops_do to iterate over an OopStorage containing weak oops.

> 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.)

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.

------------------------------------------------------------------------------
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.

------------------------------------------------------------------------------