RFR(s): 8151264: Add a notification mechanism for UL configuration changes

[David Holmes]

On 12/03/2016 12:27 AM, Robbin Ehn wrote:
> Hi, please review this updated changeset.
>
> Webrev: http://cr.openjdk.java.net/~rehn/8151264/v2/webrev/

Thanks for the updates!

Three minor nits in src/share/vm/logging/logConfiguration.hpp

+   // JavaMain thread may call this callback if there is an early 
registration

"JavaMain" is not a defined thing in the VM. I presume this means the 
main Java thread that created the VM so "The main Java thread may call ..."

+   // else the attach listener JavaThread, started via diagonstic 
command, will be executing thread.

diagonstic -> diagnostic

+   // The main purpose if this callback is to see if a loglevel have 
been changed.

purpose if -> purpose of

No need to see new webrev.

Thanks,
David

> Thanks!
>
> /Robbin
>
> On 03/10/2016 11:38 AM, Robbin Ehn wrote:
>> Thanks David for looking at this.
>>
>> On 03/10/2016 11:19 AM, David Holmes wrote:
>>> Hi Robbin,
>>>
>>> On 9/03/2016 7:02 PM, Robbin Ehn wrote:
>>>>
>>>> Hi, please review this minor change.
>>>
>>> A notification mechanism is not so minor - what are the programming
>>> rules for the callbacks? which threads might execute them? Are there any
>>> obvious deadlock/livelock potentials? Do we need to unregister a
>>> callback?
>>>
>>> At a minimum there should be some comments regarding the nature of the
>>> callback and what it can and can't do.
>>
>> Yes, I will document it better.
>>
>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8151264/
>>>> Webrev: http://cr.openjdk.java.net/~rehn/8151264/webrev/
>>>
>>> Some minor nits:
>>>
>>> src/share/vm/logging/log.cpp
>>>
>>> Copyright needs 2016 added.
>>>
>>> ---
>>>
>>> src/share/vm/logging/logConfiguration.cpp
>>>
>>> +   assert(cb != NULL, "Should not initialize register NULL as
>>> listener.");
>>>
>>> "initialize register" doesn't make grammatical sense - delete
>>> "initialize"
>>>
>>> +   for (size_t i=0;i<_n_listener_callbacks;i++) {
>>>
>>> Style nits: spaces around operators and after ;
>>>
>>> ---
>>>
>>> src/share/vm/logging/logConfiguration.hpp
>>>
>>> +   // After any configuration change this method should be called by in
>>> scope of LogConfigurationLock
>>>
>>> Delete 'by' - or may I suggest:
>>>
>>> // This should be called after any configuration change while still
>>> holding ConfigurationLock
>>>
>>
>> Yes, thanks!
>>
>> /Robbin
>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Tested with a new internal vm test (included).
>>>>
>>>> Thanks!
>>>>
>>>> /Robbin