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: [email protected]; [email protected] 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: [email protected] > Cc: [email protected] > 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
