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

[coleen.phillimore at oracle.com]

On 1/29/19 4:51 AM, Thomas Schatzl wrote:
> Hi,
>
> On Mon, 2019-01-28 at 16:46 -0500, coleen.phillimore at oracle.com wrote:
>> Hi Thomas,  Can you forward the review thread for JDK-8213229?
>    the thread starts at
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-January/024474.html
>   , but the interesting part is probably starting here
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-January/024649.html
>   .

So this moves StringTable processing into the WeakProcessor.  good! Not 
a review though.

http://cr.openjdk.java.net/~tschatzl/8217778/webrev/src/hotspot/share/gc/shared/weakProcessor.inline.hpp.frames.html

If these Counting closures are only used in the weakProcessor.cpp file, 
they should be defined there.  It looks like their use was moved.

http://cr.openjdk.java.net/~tschatzl/8217778/webrev/src/hotspot/share/gc/shared/weakProcessor.cpp.frames.html

I don't know what "is_serial does but it doesn't look like you're using 
the num_dead() count when calling processor.

  40     if (WeakProcessorPhases::is_serial(phase)) {
41 CountingIsAliveClosure<BoolObjectClosure> cl(is_alive);
   42       WeakProcessorPhases::processor(phase)(&cl, keep_alive);
   43     } else {
44 CountingSkippedIsAliveClosure<BoolObjectClosure, OopClosure> 
cl(is_alive, keep_alive);
45 WeakProcessorPhases::oop_storage(phase)->oops_do(&cl);
   46       if (WeakProcessorPhases::is_stringtable(phase)) {
47 StringTable::inc_dead_counter(cl.num_dead() + cl.num_skipped());
48 }
   49     }

Thanks,
Coleen

>> I wonder if we could just trigger_unloading after any GC has run and
>> ignore dead string counts.  Are they ever zero?
> That seems to be a waste of cpu cycles assuming that interned strings
> typically have a "long" lifetime.

Since the VM has to intern any strings in the JVM_CONSTANT_String in the 
classfile, class unloading probably should trigger_unloading 
unconditionally if any classes are unloaded.  It seems very likely there 
would be unreferenced strings.

But we may need the ServiceThread to unlink reclaimed entries even if 
classes aren't unloaded, which may be infrequent but there may have been 
stressful applications that have required it in the past.

Can you log if the count is ever too low to process the StringTable?

The change for this looks reasonable, but not a full review.  I kind of 
like keeping the concept of weak_oops_do() because it's a well known 
idiom but as Kim says, it's not necessary to call weak_oops_do on 
something that contains weak oops.

CR:
https://bugs.openjdk.java.net/browse/JDK-8217778
Webrev:
http://cr.openjdk.java.net/~tschatzl/8217778/webrev/


Thanks,
Coleen
>
> Thanks,
>    Thomas
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190129/485965b2/attachment.htm>