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

[Kim Barrett]

> On Jan 24, 2019, at 7:55 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> On Wed, 2019-01-23 at 23:07 -0500, Kim Barrett wrote:
>> 
>> 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.

OK.

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

I understood that.  It was just the comment wording that gave me trouble.
“take the time” := use time or expend effort.

The new code is good though, and addresses the comment problem.

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

OK.

> 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

Looks good.

One very minor formatting nit, for which I don’t need a new webrev.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1CollectedHeap.hpp
1336   void string_dedup_cleaning(BoolObjectClosure* is_alive,
1337                         OopClosure* keep_alive,
1338                         G1GCPhaseTimes* phase_times = NULL);

Second and third parameters are not indented consistently with other
code; should be aligned with the first parameter.

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