'Re: RFR: 8149996: TraceLoaderConstraints has been converted to Unified Logging.'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8149996: TraceLoaderConstraints has been converted to Unified Logging.
From:       Max Ockner <max.ockner () oracle ! com>
Date:       2016-03-29 17:04:24
Message-ID: 56FAB598.4010407 () oracle ! com
[Download RAW message or body]

OK I will submit this now.  Thanks everybody.
On 3/28/2016 3:01 PM, Coleen Phillimore wrote:
> Max,
> This looks good!
> Thanks,
> Coleen
> 
> On 3/28/16 1:11 PM, Max Ockner wrote:
> > Hey everyone. Patching this into a jake repo was difficult as there 
> > were other dependencies that had not yet been pulled into jake. As a 
> > result, I tabled this until jigsaw integration.
> > 
> > New webrev: http://cr.openjdk.java.net/~mockner/8149996.06/
> > - Updated the test to check for new paths.
> > 
> > Tested with jtreg runtime.
> > 
> > Thanks,
> > Max
> > 
> > On 3/16/2016 5:09 PM, David Holmes wrote:
> > > Follow ups below ...
> > > 
> > > On 17/03/2016 1:00 AM, Max Ockner wrote:
> > > > Comments below.
> > > > 
> > > > On 3/16/2016 2:11 AM, David Holmes wrote:
> > > > > Hi Max,
> > > > > 
> > > > > tl;dr: you can push this as-is (modulo fixing copyright years) but I
> > > > > have some further queries/discussion points below. :)
> > > > > 
> > > > > Meta question: should we consider using "classload, constraints"
> > > > > instead of "loaderconstraints" ?
> > > > > 
> > > > I think this would be appropriate.
> > > 
> > > Ok.
> > > 
> > > > > On 16/03/2016 1:00 AM, Max Ockner wrote:
> > > > > > Hello again,
> > > > > > 
> > > > > > new webrev: http://cr.openjdk.java.net/~mockner/8149996.05/
> > > > > > - added resource marks back in
> > > > > > - reverted to using outputStreams instead of log_info macro
> > > > > > 
> > > > > > I ran into an issue with the outputStream eating its output when I
> > > > > > removed " ]\n" from the end of some of the messages, solved by 
> > > > > > changing
> > > > > > "out->print" to "out->print_cr".
> > > > > 
> > > > > Aside: it is actually more efficient to have the \n in the string 
> > > > > than
> > > > > to use the print_cr form. :)
> > > > > 
> > > > Thanks for pointing this out. I can change it.
> > > 
> > > It was just an observation - we don't pay this much attention 
> > > elsewhere.
> > > 
> > > > > Nit: is there a reason to remove the initial capitals from the log
> > > > > messages? I find the capitals are a visual aid when trying to find 
> > > > > the
> > > > > end of the decorators.
> > > > > 
> > > > > Aside 2: I notice we have now removed the only example of a 
> > > > > product_rw
> > > > > flag. I wonder if we should rip out product_rw support? (Separate RFE
> > > > > of course). I'm also wondering if our logging flags are
> > > > > visible/modifiable via the management tools like 
> > > > > manageable/product_rw
> > > > > flags are?
> > > > > 
> > > > From an email discussion with Marcus:
> > > > "There is a DCMD to reconfigure logging during runtime. It's all baked
> > > > into a single command called VM.log and it works similar to the -Xlog
> > > > command, but all of the parameters are named, so -Xlog:gc:gclog.txt
> > > > would be VM.log output=gclog.txt what=gc."
> > > 
> > > Thanks.
> > > 
> > > > > With regard to the test I'm not sure those "loaders" are going to
> > > > > continue to be valid once Jake has integrated. So you might want to
> > > > > try this on a Jake build before finalising it.
> > > > > 
> > > > 
> > > > Do you think this is important to test before submitting?
> > > 
> > > It might stop working next week so yes I think it worth checking 
> > > this now.
> > > 
> > > Thanks,
> > > David
> > > 
> > > > > Thanks,
> > > > > David
> > > > > 
> > > > > > I thought about writing a log_info_rm which evaluates to a code 
> > > > > > block
> > > > > > instead of a function, but I think one of the benefits of the 
> > > > > > current
> > > > > > macros is that they allow two variable argument lists in the same
> > > > > > expression. Returning a write function is important for handling the
> > > > > > format string and its variable length argument list of format
> > > > > > %substitutions.
> > > > > > 
> > > > > > Thanks,
> > > > > > Max
> > > > > > 
> > > > > > 
> > > > > > On 3/9/2016 11:29 PM, David Holmes wrote:
> > > > > > > On 10/03/2016 8:03 AM, Coleen Phillimore wrote:
> > > > > > > > 
> > > > > > > > http://cr.openjdk.java.net/~mockner/8149996.02/src/share/vm/classfile/loaderConstraints.cpp.frames.html \
> > > > > > > >  
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Why did you take out the ResourceMarks at line 130, 160, 231 
> > > > > > > > and 244,
> > > > > > > > 297, 308 and 357?
> > > > > > > 
> > > > > > > Yes these should all be of the form:
> > > > > > > 
> > > > > > > if (log_is_enabled(...)) {
> > > > > > > ResourceMark rm;
> > > > > > > // log stuff
> > > > > > > }
> > > > > > > 
> > > > > > > > Unfortunately, we really need log_info_rm(tag)("String") call 
> > > > > > > > because
> > > > > > > > all those places need a ResourceMark.
> > > > > > > > 
> > > > > > > > Anytime we call name()->as_C_string() there needs to be a 
> > > > > > > > ResourceMark
> > > > > > > > in the scope of the call.
> > > > > > > 
> > > > > > > Which means that a log_info_rm() call wouldn't help because we'd 
> > > > > > > still
> > > > > > > be calling as_C_string in the caller. :(
> > > > > > > 
> > > > > > > David
> > > > > > > 
> > > > > > > > 
> > > > > > > > Coleen
> > > > > > > > 
> > > > > > > > On 3/9/16 3:47 PM, Max Ockner wrote:
> > > > > > > > > Hello again,
> > > > > > > > > 
> > > > > > > > > Please review this change. TraceLoaderConstraints has been 
> > > > > > > > > converted
> > > > > > > > > to Unified Logging and can be accessed using
> > > > > > > > > '-Xlog:loaderconstraints=info'. TraceLoaderConstraints has 
> > > > > > > > > been added
> > > > > > > > > to the logging alias table.
> > > > > > > > > 
> > > > > > > > > There are 0 lines of output for java -version, and ~10 lines from
> > > > > > > > > LoaderConstraintsTest.java
> > > > > > > > > 
> > > > > > > > > bug: https://bugs.openjdk.java.net/browse/JDK-8149996
> > > > > > > > > webrev: http://cr.openjdk.java.net/~mockner/8149996.02/
> > > > > > > > > 
> > > > > > > > > Testing: jtreg runtime. Added new test which triggers logging for
> > > > > > > > > loaderconstraints by forcing class unloading. No other 
> > > > > > > > > references to
> > > > > > > > > TraceLoaderConstraints found in hotspot/test, jdk/test , or in 
> > > > > > > > > the
> > > > > > > > > noncolo tests.
> > > > > > > > > 
> > > > > > > > > Thanks,
> > > > > > > > > Max
> > > > > > > > 
> > > > > > 
> > > > 
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic