RFR: 8149064: Convert TraceProtectionDomainVerification to Unified Logging
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Feb 26 23:20:05 UTC 2016
This looks good!
Coleen
On 2/26/16 5:18 PM, Max Ockner wrote:
> Another round of changes. In addition to completing the changes
> suggested by Coleen, I have made the test case more thorough to test
> for the distinction between debug level and trace level output.
>
> webrev: http://cr.openjdk.java.net/~mockner/protectiondomain.04/
>
> More comments below.
>
> Thanks,
> Max
>
> On 2/25/2016 6:22 PM, Coleen Phillimore wrote:
>>
>>
>> On 2/25/16 4:56 PM, Max Ockner wrote:
>>> Hello ,
>>>
>>> There do not seem to be too many opinions on this one, so I have
>>> just responded to Coleen's suggestions (comments inline).
>>> Webrev: http://cr.openjdk.java.net/~mockner/protectiondomain.03/
>>
>> You missed changing the message:
>>
>> *!_st_->print_cr("pd set = #%d", count);*
>>
>> to
>>
>> *!_st_->print_cr("pd set = #%d", count);*
>>
>
> I originally didn't see the suggested change in this line ( I thought
> you were suggesting that the old output simply be the output of a new
> function named print_count). I have changed this to
>
> *!_st_->print_cr("pd set count = #%d", count);*
>
>> You can shorten this to:
>>
>> *!if (TraceProtectionDomainVerification) {*
>> *!if (_log_is_enabled(Debug, protectiondomain)_) {*
>> *+ ResourceMark rm;*
>> *+ outputStream* log = LogHandle(protectiondomain)::debug_stream();*
>> if (HAS_PENDING_EXCEPTION) {
>> *!tty->print_cr(" -> DENIED !!!!!!!!!!!!!!!!!!!!!");*
>> *!_log->print_cr("_DENIED !!!!!!!!!!!!!!!!!!!!!");*
>> } else {
>> *!tty->print_cr(" -> granted");*
>> *!_log->print_cr("_granted");*
>> }
>> *tty->cr();*
>> } to
>> if (HAS_PENDING_EXCEPTION) {
>> *!tty->print_cr(" -> DENIED !!!!!!!!!!!!!!!!!!!!!");*
>> *!_log__debug(protectiondomain)_("_DENIED !!!!!!!!!!!!!!!!!!!!!");*
>> } else {
>> *!tty->print_cr(" -> granted");*
>> *!__log_debug(protectiondomain)_("_granted");*
>> }
>> *tty->cr();*
>> }
> Yes, now that the cr() is gone there is no need to explicitly call the
> log streams. Fantastic. This has been fixed.
>> http://cr.openjdk.java.net/~mockner/protectiondomain.03/test/runtime/logging/Hello.java.html
>> Needs a copyright. Do you still get the messages if Hello is an
>> internal class? Thanks, Coleen
> Yes. I have moved Hello into ProtectionDomainVerificationTest.java as
> an inner class.
>>> Thanks, Max On 2/24/2016 10:57 AM, Coleen Phillimore wrote:
>>>> On 2/22/16 6:04 PM, Coleen Phillimore wrote:
>>>>> Hi Max, I think this looks fine. A couple of comments though:
>>>>> http://cr.openjdk.java.net/~mockner/protectiondomain.02/src/share/vm/classfile/dictionary.hpp.udiff.html
>>>>> This is preexisting code, which is apparently giving the number of
>>>>> pd set. Not sure what the use is, but could you rename the
>>>>> function print_count() and make the output *!_st_->print_cr("pd
>>>>> set count = #%d", count);*
>>> This has been fixed.
>>>>> Maybe it wouldn't look so strange in the log.
>>>>> http://cr.openjdk.java.net/~mockner/protectiondomain.02/test/runtime/logging/ProtectionDomainVerificationTest.java.html
>>>>> The test looks like it's missing "Hello.java" but maybe you can
>>>>> use Empty.class from ClassLoadUnloadTest.java.
>>> Added Hello.java
>>>>> http://cr.openjdk.java.net/~mockner/protectiondomain.02/src/share/vm/classfile/systemDictionary.cpp.udiff.html
>>>>> You're right, I have a vote for the format - it seems like a more
>>>>> compact log would be better: [0.138s][debug][protectiondomain] -
>>>>> class loader: a 'sun/misc/Launcher$AppClassLoader' protection
>>>>> domain: a 'java/security/ProtectionDomain' loading:
>>>>> 'java/lang/Object' -> granted
>>> These messages have been condensed to a single line.
>>>> I should point out that I 've never used this logging for anything
>>>> so my opinion in favor of brevity can be easily countered if anyone
>>>> has a different opinion. Coleen
>>>>> Or at least remove the blank line in the log.
>>> The blank line has been removed.
>>>>> thanks, Coleen On 2/22/16 12:17 PM, Max Ockner wrote:
>>>>>> Please review this conversion of
>>>>>> TraceProtectionDomainverification to Unified Logging. Theres some
>>>>>> output that I think should be changed, but I want to see what
>>>>>> others think first. Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8149064 Webrev:
>>>>>> http://cr.openjdk.java.net/~mockner/protectiondomain.02/
>>>>>> TraceProtectionDomainVerification is a non-product flag, but the
>>>>>> output will now be available in product builds through
>>>>>> -Xlog:protectiondomain=debug. - The output is concise.
>>>>>> "-version" produces no output, and "Hello.java" produces 27 lines
>>>>>> (if level=trace) or fewer. See below. - This change doesn't rely
>>>>>> on "#ifndef PRODUCT" and is smaller than most logging changes
>>>>>> that we've seen so far. - refworkload shows now significant
>>>>>> performance changes. Tested with jtreg hotspot tests and
>>>>>> refworkload. Here is sample output for a run of Hello.java:
>>>>>> [0.138s][debug][protectiondomain] - class loader: a
>>>>>> 'sun/misc/Launcher$AppClassLoader'
>>>>>> [0.138s][debug][protectiondomain] - protection domain: a
>>>>>> 'java/security/ProtectionDomain'
>>>>>> [0.138s][debug][protectiondomain] - loading: 'java/lang/Object'
>>>>>> [0.139s][debug][protectiondomain] -> granted
>>>>>> [0.139s][debug][protectiondomain]
>>>>>> [0.139s][trace][protectiondomain] pd set = #1
>>>>>> [0.139s][debug][protectiondomain] Checking package access
>>>>>> [0.139s][debug][protectiondomain] - class loader: a
>>>>>> 'sun/misc/Launcher$AppClassLoader'
>>>>>> [0.139s][debug][protectiondomain] - protection domain: a
>>>>>> 'java/security/ProtectionDomain'
>>>>>> [0.139s][debug][protectiondomain] - loading: 'java/lang/String'
>>>>>> [0.139s][debug][protectiondomain] -> granted
>>>>>> [0.139s][debug][protectiondomain]
>>>>>> [0.139s][trace][protectiondomain] pd set = #1
>>>>>> [0.139s][debug][protectiondomain] Checking package access
>>>>>> [0.139s][debug][protectiondomain] - class loader: a
>>>>>> 'sun/misc/Launcher$AppClassLoader'
>>>>>> [0.139s][debug][protectiondomain] - protection domain: a
>>>>>> 'java/security/ProtectionDomain'
>>>>>> [0.139s][debug][protectiondomain] - loading: 'java/lang/System'
>>>>>> [0.139s][debug][protectiondomain] -> granted
>>>>>> [0.139s][debug][protectiondomain]
>>>>>> [0.139s][trace][protectiondomain] pd set = #1
>>>>>> [0.140s][debug][protectiondomain] Checking package access
>>>>>> [0.140s][debug][protectiondomain] - class loader: a
>>>>>> 'sun/misc/Launcher$AppClassLoader'
>>>>>> [0.140s][debug][protectiondomain] - protection domain: a
>>>>>> 'java/security/ProtectionDomain'
>>>>>> [0.140s][debug][protectiondomain] - loading:
>>>>>> 'java/io/PrintStream' [0.140s][debug][protectiondomain] ->
>>>>>> granted [0.140s][debug][protectiondomain]
>>>>>> [0.140s][trace][protectiondomain] pd set = #1 I am anticipating
>>>>>> several comments and suggestions, but I believe there are only
>>>>>> minor changes remaining. I would like to remove the empty space,
>>>>>> though I believe this was present in the
>>>>>> TraceProtectionDomainVerification output to make the output more
>>>>>> readable. I also anticipate that you may want me to remove the
>>>>>> "-" from in front of some of the lines. Again, I would like
>>>>>> input since output seems like it was designed to look a certain
>>>>>> way. Thanks, Max
More information about the hotspot-runtime-dev
mailing list