RFR: JDK-8143255: Remove debug logging from SymbolTable::unlink() and SymbolTable::possibly_parallel_unlink() (original) (raw)

Bengt Rutisson bengt.rutisson at oracle.com
Thu Nov 19 15:23:10 UTC 2015

On 2015-11-19 16:00, Coleen Phillimore wrote:

Thanks Bengt for the reminder. I did add this code when moving Symbols out of PermGen. lol The GC people did not want to see this output so I added Verbose and WizardMode for extra good measure. I have never looked at this output again. I think it can be removed. There are similar statistics in PrintSymbolTableSizeHistogram that are more useful and less noisy. Webrev .01 looks good.

Thanks, Coleen! I'll go ahead and push this now.

Bengt

For UL conversion, yes, absolutely, we are looking at removing some "logging" that is actually left-over debugging that may or may not help a new person debugging the code. In this case, it's better not to have this clutter. It's costly to maintain and have to look at in such a large (getting larger) codebase. Thanks, Coleen On 11/19/15 3:13 AM, Bengt Rutisson wrote:

On 2015-11-19 08:54, David Holmes wrote: On 19/11/2015 5:41 PM, Bengt Rutisson wrote:

Hi David, On 2015-11-18 22:42, David Holmes wrote: Hi Bengt,

On 19/11/2015 5:47 AM, Bengt Rutisson wrote:

Hi everyone, Could I have a couple of reviews for this patch to remove some debugging code? http://cr.openjdk.java.net/~brutisso/8143255/webrev.00/ https://bugs.openjdk.java.net/browse/JDK-8143255 From the bug report: The logging inside SymbolTable::unlink() and SymbolTable::possiblyparallelunlink() is guarded by Verbose and WizardMode, which are both develop flags. This means that the log output is not available in product builds. So, this logging is most likely just leftover debugging code. As this is primarily guarded by PrintGCDetails the GC team owns this as far as I am concerned and so they can do as they desire. However ... This is runtime code, so I think it was a mistake to guard this logging with PrintGCDetails. But since I am actively working on replacing the GC logging with the new unified logging framework I stumbled across this piece of code. Well someone obviously considered this of interest when doing GC logging :) It was added by the runtime team in this change: "Use native memory and reference counting to implement SymbolTable" https://bugs.openjdk.java.net/browse/JDK-6990754 See: http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/3582bf76420e#l68.89 And then changed by this change: "Symbol printouts shouldnt be under PrintGCDetails" https://bugs.openjdk.java.net/browse/JDK-7024584 See: http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/df1347358fe6 So, I would say no. This was never of interest for GC logging. I discussed it with the runtime team and they did not consider the logging useful. So, rather than trying to figure out a conversion to the new logging framework it seemed better to remove it.

Most, if not all, non-product logging code could be characterized as "leftover debugging code" - but that doesn't mean it should all be ripped out. The use of Verbose && WizardMode does seem a little excessive in this case - one should suffice. As documented the aim is to ensure that this logging, while logically wanted as part of PrintGCDetails, is not included in the product mode because people parse that logging information. In the GC team we don't use this logging (even though it is for some reason guarded by PrintGCDetails too) and I discussed it with Coleen and Rachel in the Runtime team. The Runtime team also don't use the logging. This kind of logging is used to expose additional details while testing/debugging issues. It isn't expected to be used on a regular basis, and may well have not been used for years, or may have last been used extensively by people no longer working on OpenJDK. Agreed. And unused code is just noise in the code base, so it is nice to get it cleaned up. But on that basis the bulk of non-product logging code would be a candidate for deletion. I think useful logging should be left (and preferably made available in product mode) and useless logging should be removed.

It is pretty straight forward to add this logging back if it is ever needed for a debugging session. Until then we should remove this from the code base. Mostly likely someone would not even know it ever existed and would simply re-write a new piece of logging code - which might even get checked in. And the cycle repeats. ;-) That's my point. Not that the cycle repeats, but that it is very easy to implement this code again if it would ever be needed. The benefit of having it checked in is very little compared to the negative effect on the reading speed for all developers that have to read this code. Something to consider on a case by case basis I hope. Else, as I said, a lot of non-product logging should go. Definitely a case by case thing! We just have to remember that there is a significant cost to having useless code just laying around. Cheers, Bengt Cheers, David Given the trend elsewhere I'm a little surprised this isn't simply being included in the product mode logging. :) In my opinion what we are trying do do elsewhere is to say that non-product logging should be used only for performance reasons or when we worry about the memory footprint. Most of the time we want the logging to be available in product builds if it in any way produces useful information. In this case it is not producing useful information in product or non-product builds, so there is no reason to move it to product level logging. Cheers, Bengt Cheers, David Thanks, Bengt

More information about the hotspot-runtime-dev mailing list