RFR: 8139564: Convert TraceDefaultMethods to Unified Logging

Fri Oct 23 21:56:34 UTC 2015

Copying Mattis.   Mattis, please include anyone from sustaining that 
could have feedback on the runtime conversion to the Unified Logging 
framework.
Thanks!
Coleen

On 10/23/15 4:02 PM, Karen Kinnear wrote:
> Rachel,
>
> Nice job!
>
> After discussion with you, Max and Coleen, I wanted to mention that in addition to doing the code review,
> since this is an early example, we also wanted feedback on the approach.
>
> A couple of thoughts in terms of product vs. develop tradeoffs
>
> I do not think we want to move all tracing flag output to product mode, I think we will want to have each review
> look closely at whether the information would be useful for the customer.
>
> 1. Customer usefulness
> Mattis Castegren and the sustaining team would have good advice here, e.g. Matts would know
> better if jdk8 customers have had issues with default methods. I personally think this one is useful
> customer output, but he would have a better perspective.
>
> 2. Performance.
>   I totally agree if you run without logging on and see a performance problem that you
> would not turn the log on for product.
>
> I really like the idea of having two levels of develop flags - the current “verbose” information makes
> a good candidate for that.
>
> Worth asking Mattis what would be considered a significant performance problem if you were to
> turn logging on for a given level.
>
> 3. footprint - for embedded and other small platforms
> Footprint really matters. So moving code from develop to product might be small for each flag, but
> might add up. So it is worth considering in the trade-offs. Note we also have conditionals for the
> minimal vm if needed.
>
> Code review:
> 1) tests run
> — when you say RBT, that doesn’t tell me what tests you chose to run, so please list testlists
> — I will assume you ran the defmeth tests? If not, please do.
>
> 2) The test you created assumes the strings in the source don’t change. Did you (or could you)
> possibly add a comment to the sources that there is a test that assumes this? At some point in
> the future I would expect that the number of tests we add will grow so large that some may
> be moved to later runs.
>
> Code:
> 1. print_sig_on
> It appears that no one calls print_sig_on. I suspect I left it in there accidentally when removing
> other functionality. Could you possibly remove it?
>
> 2) tty-> in print_selected, I presume you want to replace the tty
>
> thanks,
> Karen
>
>
>> On Oct 20, 2015, at 1:53 PM, Rachel Protacio <rachel.protacio at oracle.com> wrote:
>>
>> Thanks, Harold!
>> Rachel
>>
>> On 10/20/2015 10:52 AM, harold seigel wrote:
>>> Hi Rachel,
>>>
>>> The changes look good.
>>>
>>> Thanks, Harold
>>>
>>> On 10/19/2015 5:02 PM, Rachel Protacio wrote:
>>>> Please see updated webrev http://cr.openjdk.java.net/~rprotacio/8139564.02/ with the following changes:
>>>> - repositioned and deleted ResourceMark's, as per Harold's suggestions
>>>> - fixed copyright year in test file
>>>> - moved update_position() line in ostream.cpp because it was breaking indentation in defaultmethods logging
>>>>
>>>> Marcus, see reply inline.
>>>>
>>>> On 10/19/2015 10:35 AM, Marcus Larsson wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 2015-10-16 18:21, Coleen Phillimore wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I added the serviceability group so they can comment on this as well.   I think having logging in the PRODUCT build is requested so that we can more easily debug customer problems.   That said, we will not enable logging in product if we see any performance problem.  Also for some options it's possible that these are strictly internal debugging options and in that case we'll either remove them if they're no longer useful, or make them Develop level options.
>>>>> This seems like a good approach to me. The develop level was added to accommodate internal or performance sensitive logging that shouldn't be included in the product.
>>>>>> Printing default methods seems to be something that might be borderline in the second case, but we've decided to make it product level logging.   We could change our minds about this though, so your comments are welcome.
>>>>> If it's borderline to internal wouldn't it be more fitting to use trace level for this logging?
>>>> Since it's not a question of verbosity but of audience, we'll leave it as it is.
>>>>
>>>> Thanks,
>>>> Rachel
>>>>> Thanks,
>>>>> Marcus
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>> On 10/15/15 6:33 PM, Ioi Lam wrote:
>>>>>>>
>>>>>>> On 10/15/15 10:51 AM, Rachel Protacio wrote:
>>>>>>>> Hi, Ioi,
>>>>>>>>
>>>>>>>> Thanks for the comments. While all valid points, the decision by the serviceability team with regards to the logging framework as a whole is to move all the output to product mode. Because of this, I ran performance tests to make sure that the newly-introduced product code will not slow it down. So yes, all the "#ifndef PRODUCT" sections that are necessary for this logging have been liberated to product mode.
>>>>>>>>
>>>>>>> Thanks Rachel. This makes sense.
>>>>>>>
>>>>>>> - Ioi
>>>>>>>
>>>>>>>> Also, I realized I did not remove the TraceDefaultMethods flag from globals.hpp, so here is the link to the updated webrev: http://cr.openjdk.java.net/~rprotacio/8139564.01/
>>>>>>>> Which builds appropriately. The change now encompasses all the references to TraceDefaultMethods. A compatibility request has been accepted with regards to this change.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Rachel
>>>>>>>>
>>>>>>>> On 10/14/2015 11:58 PM, Ioi Lam wrote:
>>>>>>>>> Hi Rachel,
>>>>>>>>>
>>>>>>>>> Before your changes, this block of code  would be excluded from product builds:
>>>>>>>>>
>>>>>>>>> 684 #ifndef PRODUCT
>>>>>>>>> 685   if (TraceDefaultMethods) {
>>>>>>>>> 686     tty->print_cr("Slots that need filling:");
>>>>>>>>> 687     streamIndentor si(tty);
>>>>>>>>> 688     for (int i = 0; i < slots->length(); ++i) {
>>>>>>>>> 689       tty->indent();
>>>>>>>>> 690       slots->at(i)->print_on(tty);
>>>>>>>>> 691       tty->cr();
>>>>>>>>> 692     }
>>>>>>>>> 693   }
>>>>>>>>> 694 #endif // ndef PRODUCT
>>>>>>>>>
>>>>>>>>> but after your change, it will be included in product builds. This means product builds will have more verbose output than before. It also means that the product builds will get bigger (because some printing code, such as EmptyVtableSlot::print_on(), would need to be enabled for product builds as well).
>>>>>>>>>
>>>>>>>>> I am not very familiar with UL so maybe this is an FAQ ... while doing the UL conversion, should we add all the old "ifndef PRODUCT" logs into the product build?
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> - Ioi
>>>>>>>>>
>>>>>>>>> On 10/14/15 7:10 PM, Rachel Protacio wrote:
>>>>>>>>>> Hello! Please take a look at my enhancement, the first of the runtime logging flags to be converted.
>>>>>>>>>>
>>>>>>>>>> Summary: The former -XX:+TraceDefaultMethods flag is updated to the unified logging framework and is now replaced with -Xlog:defaultmethods.
>>>>>>>>>>
>>>>>>>>>> open webrev: http://cr.openjdk.java.net/~rprotacio/8139564/
>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8139564
>>>>>>>>>> testing: Passes JPRT, RBT, and RefWorkload performance testing.
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Rachel