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

[Bengt Rutisson]

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