[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8145083: Use semaphore instead of mutex for synchronization of Unified Logging configuratio
From:       Marcus Larsson <marcus.larsson () oracle ! com>
Date:       2015-12-17 10:42:46
Message-ID: 567291A6.9070801 () oracle ! com
[Download RAW message or body]

Thanks for reviewing, David!

On 2015-12-17 06:34, David Holmes wrote:
> 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@openjdk.java.net
>>> Cc: hotspot-runtime-dev@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
>>

[prev in list] [next in list] [prev in thread] [next in thread]