Bug 199832 – [bmalloc] Add option to use pthread mutex instead of spin lock.

• Component: Platform
• Status: RESOLVED INVALID

[Basuke Suzuki]

Our platform prefer using actual mutex.

[Basuke Suzuki]

Created attachment 374222 [details]
PATCH

[Keith Miller]

Comment on attachment 374222 [details]
PATCH

If we are going to support this we should try to minimize the usage of #if BUSE(PTHREAD_MUTEX). e.g. The constructor should exist everywhere but only do things for pthreads, try_lock could be pthread_mutex_trylock, etc.

[Basuke Suzuki]

Created attachment 374230 [details]
PATCH

[Basuke Suzuki]

(In reply to Keith Miller from comment #2)
> Comment on attachment 374222 [details]
> PATCH
> 
> If we are going to support this we should try to minimize the usage of #if
> BUSE(PTHREAD_MUTEX). e.g. The constructor should exist everywhere but only
> do things for pthreads, try_lock could be pthread_mutex_trylock, etc.

Do you mean like this? I removed the differences between both implementation as much as possible. For constructor, we need a non constexpr version so that I kept that difference as is.

[EWS Watchlist]

Attachment 374230 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Mutex.h:111:  Mutex::try_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 374230 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=374230&action=review

> Source/bmalloc/bmalloc/Mutex.h:46
> +#if BUSE(PTHREAD_MUTEX)
> +    Mutex();
> +#else

This is not necessary. PTHREAD_MUTEX_INITIALIZER can initialize it under the constexpr context.

> Source/bmalloc/bmalloc/Mutex.h:92
> +#if BUSE(PTHREAD_MUTEX)
> +inline Mutex::Mutex()
>  {
> -    return !m_flag.exchange(true, std::memory_order_acquire);
> +    pthread_mutex_init(&m_mutex, nullptr);
> +}
> +#endif

pthread_mutex_init and Mutex constructor is not necessary because we initialize it via PTHREAD_MUTEX_INITIALIZER.

[Basuke Suzuki]

Created attachment 374232 [details]
TRY WITH NO constexpr

[Basuke Suzuki]

(In reply to Yusuke Suzuki from comment #6)
> Comment on attachment 374230 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374230&action=review
> 
> > Source/bmalloc/bmalloc/Mutex.h:46
> > +#if BUSE(PTHREAD_MUTEX)
> > +    Mutex();
> > +#else
> 
> This is not necessary. PTHREAD_MUTEX_INITIALIZER can initialize it under the
> constexpr context.
> 
> > Source/bmalloc/bmalloc/Mutex.h:92
> > +#if BUSE(PTHREAD_MUTEX)
> > +inline Mutex::Mutex()
> >  {
> > -    return !m_flag.exchange(true, std::memory_order_acquire);
> > +    pthread_mutex_init(&m_mutex, nullptr);
> > +}
> > +#endif
> 
> pthread_mutex_init and Mutex constructor is not necessary because we
> initialize it via PTHREAD_MUTEX_INITIALIZER.

Ah, okay, that's good to know. But we do still need destructor for cleanup, right? I thought the error below was caused by that.

/Volumes/Data/EWS/WebKit/Source/bmalloc/bmalloc/CryptoRandom.cpp:79:1: error: declaration requires a global destructor [-Werror,-Wglobal-constructors]
In file included from /Volumes/Data/EWS/WebKit/Source/bmalloc/bmalloc/CryptoRandom.cpp:36:
/Volumes/Data/EWS/WebKit/Source/bmalloc/bmalloc/StaticPerProcess.h:103:57: note: expanded from macro 'DEFINE_STATIC_PER_PROCESS_STORAGE'
    Mutex StaticPerProcessStorageTraits<Type>::Storage::s_mutex { }; \
                                                        ^

[EWS Watchlist]

Attachment 374232 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/Mutex.h:107:  Mutex::try_lock is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 374230 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=374230&action=review

>>> Source/bmalloc/bmalloc/Mutex.h:46
>>> +#else
>> 
>> This is not necessary. PTHREAD_MUTEX_INITIALIZER can initialize it under the constexpr context.
> 
> Ah, okay, that's good to know. But we do still need destructor for cleanup, right? I thought the error below was caused by that.
> 
> /Volumes/Data/EWS/WebKit/Source/bmalloc/bmalloc/CryptoRandom.cpp:79:1: error: declaration requires a global destructor [-Werror,-Wglobal-constructors]
> In file included from /Volumes/Data/EWS/WebKit/Source/bmalloc/bmalloc/CryptoRandom.cpp:36:
> /Volumes/Data/EWS/WebKit/Source/bmalloc/bmalloc/StaticPerProcess.h:103:57: note: expanded from macro 'DEFINE_STATIC_PER_PROCESS_STORAGE'
>     Mutex StaticPerProcessStorageTraits<Type>::Storage::s_mutex { }; \
>                                                         ^

Yes, but this error is related to existence of destructor, not related to constexpr constructor.

[Geoffrey Garen]

Comment on attachment 374232 [details]
TRY WITH NO constexpr

Why should we do this? Modes are bad, so we should only add a mode if we have a really good reason.

[Basuke Suzuki]

(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 374232 [details]
> TRY WITH NO constexpr
> 
> Why should we do this? Modes are bad, so we should only add a mode if we
> have a really good reason.

Well, I cannot tell the detail..., please guess. Any way, we didn't notice there're exit-time-destructor issues on our patch and it should be solved anyway. We'll think about that later and invalidate this bug at this moment.

Thanks.

[Geoffrey Garen]

If you do choose to re-upload this change, I think we will need some way to discuss its benefits and consider alternatives, even if some of the specifics need to be confidential.

[Basuke Suzuki]

Thanks, Geoffrey.