RFR: 8139564: Convert TraceDefaultMethods to Unified Logging

Fri Oct 30 15:34:37 UTC 2015

Updated webrev at http://cr.openjdk.java.net/~rprotacio/8139564.03.

Please see replies inline. Thank you, Karen, for your extensive review! 
And thank you, Coleen for the review last night!

On 10/23/2015 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.
Because of Mattis' input, we wanted to stick with the specification and 
use them as product flags now, and if we decide we want them as develop 
flags, we can file an RFE and fix it after feature-freeze as a bug fix. 
Since you and Mattis think this might be useful in a customer situation, 
we propose to leave default methods in product mode.
> 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.
I have this staged in a repository just in case we need this for this or 
future logging flags.
> Worth asking Mattis what would be considered a significant performance problem if you were to
> turn logging on for a given level.
Since default methods logging is on debug and trace levels, we agree 
with Mattis that the customer would be willing to accept a performance 
loss for the logging itself if they need to turn on the flag for debugging.
> 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.
Ok, thanks. Since this one is so small we can leave it for now.
> 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.
I've just run the defmeth tests and hotspot/test tests. All pass!
> 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.
I did not, but it would add a lot of extra lines to the code to do this 
for each part in the tests. For this tag, it would be 8 separate places 
interspersed in the code, and future options would have similar numbers 
of comments, which could be disruptive to someone trying to read the 
code. I was instead just going to add a comment for the logging that is 
done in sub-functions to make it clear they were used in the test, but 
they turned out not to be used in this test because there was no 
hard-coded text in them. As a general rule and for future options, I'd 
propose to add a comment indicating test use only in the cases where 
logging is done in a function that takes an outputStream as input, e.g., 
where it is not obvious that the text goes to logging. How does that sound?
> 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
Good catch on both counts. I'm surprised the tty stream has been sitting 
there alongside the outputStream this whole time. I made both changes.

Thank you very much,
Rachel
> 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 webrevhttp://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