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

[Robbin Ehn]

Thanks David!

Will update accordingly.

/Robbin

On 03/14/2016 03:06 AM, David Holmes wrote:
> 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