RFR (L): 8213229: Investigate treating StringTable as weak in young collections

[Thomas Schatzl]

Hi,

On Wed, 2019-01-23 at 23:07 -0500, Kim Barrett wrote:
> > On Jan 22, 2019, at 5:02 AM, Thomas Schatzl <
> > thomas.schatzl at oracle.com> wrote:
> > 
> > Hi all,
> > 
> > On Sat, 2019-01-12 at 04:52 -0500, Kim Barrett wrote:
> > > > On Jan 11, 2019, at 9:51 PM, Kim Barrett <
> > > > kim.barrett at oracle.com>
> > > > wrote:
> > > > 
> > > > > On Jan 10, 2019, at 3:46 PM, Thomas Schatzl <
> > > > > thomas.schatzl at oracle.com> wrote:
> > > > > […]
> > > > > CR:
> > > > > https://bugs.openjdk.java.net/browse/JDK-8213229
> > > > > Webrev:
> > > > > http://cr.openjdk.java.net/~tschatzl/8213229/webrev.1/
> > > > > Testing:
> > > > > hs-tier1-6, performance checking with our performance suite
> > > > > showed no particular performance differences before/after
> > 
> > Updated webrev at 
> > 
> > http://cr.openjdk.java.net/~tschatzl/8213229/webrev.1_to_2 (diff)
> > http://cr.openjdk.java.net/~tschatzl/8213229/webrev.2/ (full)
> > 
> > implementing the notification of the ServiceThread and applying
> > Kim's naming suggestion.
> > 
> > Thanks,
> >  Thomas
> 
> General question: There are various places that now unconditionally
> report or log information about various string dedup passes, even
> when string dedup is disabled. I've questioned at least some of them
> below, but it's so common that now I'm wondering if perhaps this is
> intentional?

I was already prototyping a change that moved more work into the
PartialCleaningTask, and backed it out before proposing the change.
Naturally it would have unconditional timing of the whole phase (like
suggested e.g. in JDK-8040006 - including e.g. WeakProcessor handling
and the card table clear in that phase too). When removing these
changes, I kept parts of what I found somewhat useful to have - like
always reporting (total) timing for this phase.

Reconsidering, I think we should not cram this into this change. There
is a new webrev that removes all that.

> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> 3397     // Ideally we would use a StringDedupCleaningTask here, but
> since we want to
> 3398     // take the time we need to copy the code here.
> 
> Maybe s/take the time/record the time/ ?
> 
> I found the current wording confusing.

This code is a duplicate of StringDedupCleaningTask (which ultimately
calls StringDedup::parallel_unlink, which calls these two
unlink_or_oops_do methods); however, I did not want to mess with the
StringDedup* code introducing parameters for passing a container to
hold the various subtimings here.

How to collect timing information across gangtasks should imho be a
different discussion. I would like to at least try to avoid having a
separate "StringDedupTiming" class just for string dedup like done so
(too?) often already (ref proc, weak processor, ...)

That's why the change basically copy&pastes the code from
StringDedup::parallel_unlink() here. I hope the new code is more
understandable.


[...]
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1CollectedHeap.hpp
> 1343   // Complete cleaning used when class unloading is enabled.
> 1344   // Cleans out all structures handled by partial_cleaning and
> also the CodeCache.
> 1345   void complete_cleaning(BoolObjectClosure* is_alive, bool
> class_unloading_occurred);
> 
> [pre-existing] Comment here seems incorrect, since complete_cleaning
> also includes KlassCleaningTask.

Fixed.

[...]

> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/shared/weakProcessor.inline.hpp
>   51     bool result = _inner->do_object_b(obj);
>   52     _num_dead += !result;
> 
>   57   size_t num_dead() const { return _num_dead; }
> 
> This calculates the number of recently deceased, and does not include
> entries that were nulled out by some previous collection but not yet
> removed by the service thread.  But the calculations based on:
> 
>   89       StringTable::inc_dead_counter(cl.num_dead());
> 
> want the number of nulled out entries.
> 
> I don't see a convenient way to obtain the number that should be
> passed to inc_dead_counter here, at least not while using
> weak_oops_do.  What I think needs to be passed to inc_dead_counter is
> the number of previously nulled entries + the number of newly dead
> entries.  Using oops_do rather than weak_oops_do would allow getting
> the missing number.
> 
> The serial WeakProcessor has the same problem.
>
> We talked about this offline last week, and I think we got it wrong.
> I was working with what I remembered of that part of the StringTable
> API, and it looks like my memory was faulty.  Sorry about that.

>From what I understand this is actually an existing bug - the old code
(in StringTable) also directly set StringTable::_uncleaned_items_count
as returned by the count of the IsAliveClosure in weak_oops_do().

I would prefer fixing this separately as I believe the exact way to do
this best will be another discussion. I can file a CR for that.

New webrevs at:
http://cr.openjdk.java.net/~tschatzl/8213229/webrev.2_to_3/ (diff)
http://cr.openjdk.java.net/~tschatzl/8213229/webrev.3/ (full)
Testing:
locally running gc/g1, started hs-tier1-5

Thanks,
  Thomas