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

[David Holmes]

Looks good!

Thanks,
David

On 16/12/2015 1:12 AM, Marcus Larsson wrote:
> 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
>