RFR: 8139564: Convert TraceDefaultMethods to Unified Logging
Rachel Protacio
rachel.protacio at oracle.com
Fri Oct 30 18:22:23 UTC 2015
Thank you, Karen!
Rachel
On 10/30/2015 1:29 PM, Karen Kinnear wrote:
> Rachel,
>
> Looks really good.
>
> Thank you for the additional testing and for your thoughtful replies
> to my earlier comments/questions.
>
> thanks,
> Karen
>
>> On Oct 30, 2015, at 11:34 AM, Rachel Protacio
>> <rachel.protacio at oracle.com <mailto:rachel.protacio at oracle.com>> wrote:
>>
>> Updated webrev athttp://cr.openjdk.java.net/~rprotacio/8139564.03
>> <http://cr.openjdk.java.net/%7Erprotacio/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.
> Thanks - I think you will find that useful.
>>> 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.
> Sounds good.
>>> 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!
> Glad to hear it - and thank you.
>>> 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?
> You will have more experience with this than I will, so we’ll follow
> your judgement on that one.
>>> 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,
> Karen
>
>>
>> Thank you very much,
>> Rachel
>>> thanks,
>>> Karen
>>>
>>>
>>>> On Oct 20, 2015, at 1:53 PM, Rachel
>>>> Protacio<rachel.protacio at oracle.com
>>>> <mailto: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/
>>>>>> <webrevhttp://cr.openjdk.java.net/%7Erprotacio/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/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Erprotacio/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/
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Erprotacio/8139564/>
>>>>>>>>>>>> bug:https://bugs.openjdk.java.net/browse/JDK-8139564
>>>>>>>>>>>> <http://bugs.openjdk.java.net/browse/JDK-8139564>
>>>>>>>>>>>> testing: Passes JPRT, RBT, and RefWorkload performance testing.
>>>>>>>>>>>>
>>>>>>>>>>>> Thank you,
>>>>>>>>>>>> Rachel
>
More information about the hotspot-runtime-dev
mailing list