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

[Kim Barrett]

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

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

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1CollectedHeap.hpp
1335   // Partial cleaning of VM internal data structures.
1336   // Let the caller choose what structures to clean out:
1337   // - StringDeduplication structures
1338   void partial_cleaning(BoolObjectClosure* is_alive,
1339                         OopClosure* keep_alive,
1340                         bool process_dedup_table,
1341                         G1GCPhaseTimes* phase_times = NULL);

Now that there's just the one data structure involved, maybe this
should be clean_dedup_table() or something like that?  Unless you
think there might be some more things to be added later?

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

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
3962   {
3963     double cleaning_start = os::elapsedTime();
3965     partial_cleaning(&is_alive,
3966                      &keep_alive,
3967                      G1StringDedup::is_enabled() /* process_dedup_table */,
3968                      g1_policy()->phase_times());
3969 
3970     double cleaning_time_ms = (os::elapsedTime() - cleaning_start) * 1000.0;
3971     g1_policy()->phase_times()->record_string_deduplication_time(cleaning_time_ms);
3972   }

This is now recording a time even if dedup is not enabled, where the
old code made the whole thing, including time-recording, conditional
on dedup being enabled.  But in the old code it was manifestly obvious
we were only dealing with dedup, while here it's somewhat hidden.  It
seems wrong to assume that the only contributor to "partial_cleaning"
time is (conditional) dedup.  But see earlier comment about the
partial_cleaning name.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1FullCollector.cpp
 228     GCTraceTime(Debug, gc, phases) debug("Phase 1: String Dedup Cleanup", scope()->timer());
 229     // If no class unloading just clean out string deduplication data.
 230     _heap->partial_cleaning(&_is_alive, NULL, G1StringDedup::is_enabled());

Again here we're reporting time for a possibly disabled operation.
The old code unconditionally also processed the string table, so
unconditionally reporting the time was fine.  And again we're assuming
the only contributor to partial_cleaning time is (conditional) dedup.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp
 431   debug_time("String Deduplication", _cur_string_deduplication_time_ms);
 432   if (G1StringDedup::is_enabled()) {

Why is dedup time now reported unconditionally?

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

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