RFR: 8145083: Use semaphore instead of mutex for synchronization of Unified Logging configuration

[Markus Gronlund]

Looks good, thanks again for doing all of this.

I will be quiet now :-)

/Markus

-----Original Message-----
From: Marcus Larsson 
Sent: den 15 december 2015 16:13
To: Markus Gronlund
Cc: hotspot-runtime-dev at openjdk.java.net; serviceability-dev at openjdk.java.net
Subject: Re: RFR: 8145083: Use semaphore instead of mutex for synchronization of Unified Logging configuration

Hey Markus,

On 2015-12-15 11:58, Markus Gronlund wrote:
> Hi Marcus,
>
> Looks good!
>
> One minor thing that you don't need to fix if you don't want to:
>
> I would use:
>
> class ConfigurationLock {
> ...
>
>    debug_only(static bool current_thread_has_lock();)
>
> };
>
> to declare the this function in the class, then I would move the definition outside of it (since it's sits inside ASSERT macros).
>
> #ifdef ASSERT
>    bool ConfigurationLock::current_thread_has_lock() {
>      return _locking_thread_id == os::current_thread_id();
>    }
> #endif
>
>
> I just find it easier to read the class declaration without the macros inside of the class declaration (and since this is debug code, there is no real use of defining inline). As I said, you don’t need to fix this and to post another webrev.

I think it's worth another round. :)

New webrev:
http://cr.openjdk.java.net/~mlarsson/8145083/webrev.04/

Incremental:
http://cr.openjdk.java.net/~mlarsson/8145083/webrev.03-04/

Thanks,
Marcus


>
> Good work!
>
> /Markus
>
>
>   
>
> -----Original Message-----
> From: Marcus Larsson
> Sent: den 15 december 2015 11:25
> To: serviceability-dev at openjdk.java.net
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR: 8145083: Use semaphore instead of mutex for 
> synchronization of Unified Logging configuration
>
> Hi,
>
> New webrev:
> http://cr.openjdk.java.net/~mlarsson/8145083/webrev.03/
>
> Incremental:
> http://cr.openjdk.java.net/~mlarsson/8145083/webrev.02-03/
>
> ConfigurationLocker renamed to ConfigurationLock to avoid confusion with the MutexLocker family of classes.
> Semaphore and locking thread id moved into the ConfigurationLock; added a function to check if current thread has the lock when asserts are enabled.
>
> Regards,
> Marcus
>
>
> On 2015-12-14 16:30, Marcus Larsson wrote:
>> Hi again,
>>
>> Made some changes to the patch after feedback from Markus Grönlund.
>> The ConfigurationLocker and the semaphore have been moved out of the 
>> LogConfiguration.hpp and into the .cpp instead. The lock is only used 
>> internally by the LogConfiguration so this makes sense. It simplifies 
>> the LogConfiguration interface and hides the details of the locking 
>> under the hood.
>>
>> New webrev:
>> http://cr.openjdk.java.net/~mlarsson/8145083/webrev.02/
>>
>> Incremental:
>> http://cr.openjdk.java.net/~mlarsson/8145083/webrev.01-02/
>>
>> Thanks,
>> Marcus
>>
>> On 2015-12-14 15:53, Marcus Larsson wrote:
>>> Hi,
>>>
>>> New webrev:
>>> http://cr.openjdk.java.net/~mlarsson/8145083/webrev.01/
>>>
>>> Incremental:
>>> http://cr.openjdk.java.net/~mlarsson/8145083/webrev.00-01/
>>>
>>> Changes:
>>> * Introduced the ConfigurationLocker class for automatic wait/signal 
>>> in constructor/destructor just like a MutexLocker.
>>> * Added an assert to verify that the "lock" is held by the current 
>>> thread when calling configure_output.
>>> * Made the config-string functions in LogOutput protected and 
>>> LogConfiguration a friend of LogOutput to prevent incorrect usage of 
>>> these functions. These functions should typically only be used 
>>> inside configure_output, which now ensures that the lock is held.
>>>
>>> Thanks,
>>> Marcus
>>>
>>>
>>> On 2015-12-14 11:13, Marcus Larsson wrote:
>>>> Hi,
>>>>
>>>> Please review the following patch to use a semaphore instead of a 
>>>> mutex for the synchronization of log configuration. Using a mutex 
>>>> requires some parts of the VM to be initialized, whereas the 
>>>> semaphores can be used right from the start. This simplifies the 
>>>> code and allows very early log configuration without special cases 
>>>> for early configuration vs reconfiguration after VM init.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~mlarsson/8145083/webrev.00/
>>>>
>>>> Issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-8145083
>>>>
>>>> Thanks,
>>>> Marcus