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

[Bengt Rutisson]

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::possibly_parallel_unlink() 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.

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.

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

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