Bug 156771 – Stop using OSAtomicIncrement32Barrier

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: New Bugs

[Alex Christensen]

Stop using OSAtomicIncrement32Barrier

[Alex Christensen]

Created attachment 276781 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


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

[Alex Christensen]

Created attachment 276782 [details]
Patch

[Darin Adler]

Comment on attachment 276782 [details]
Patch

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

> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:211
> +    std::atomic_fetch_add(&m_timeBoundsQueuePtr, 1);

This is OK, but it‘s not important to use that function.

I suggest just writing this instead:

    ++m_timeBoundsQueuePtr;

Because pre-increment is guaranteed to be atomic.

Or even:

    m_timeBoundsQueuePtr = nextPtr;

because the code already relies on the value not being bumped inside the function. With some thought we might even be able to use post-increment above where the value is used to set up nextPtr;

[Darin Adler]

Comment on attachment 276782 [details]
Patch

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

>> Source/WebCore/platform/audio/mac/CARingBuffer.cpp:211
>> +    std::atomic_fetch_add(&m_timeBoundsQueuePtr, 1);
> 
> This is OK, but it‘s not important to use that function.
> 
> I suggest just writing this instead:
> 
>     ++m_timeBoundsQueuePtr;
> 
> Because pre-increment is guaranteed to be atomic.
> 
> Or even:
> 
>     m_timeBoundsQueuePtr = nextPtr;
> 
> because the code already relies on the value not being bumped inside the function. With some thought we might even be able to use post-increment above where the value is used to set up nextPtr;

pre-increment of std::atomic, I meant, not all pre-increment

[Ahmad Saleem]

Checking via BugID, this change didn't landed but the changes are already done:

https://github.com/WebKit/WebKit/blob/1006a4f24035d631a7dde6504088aacda806bb29/Source/WebCore/platform/audio/cocoa/CARingBuffer.h#L88

Slightly different from the patch:

https://github.com/WebKit/WebKit/blob/79cad6dee0909e825ce0588f132a7255dff52e41/Source/WebCore/platform/audio/cocoa/CARingBuffer.cpp#L316

and header <atomic> is not added.

Do we need to do anything more here? Thanks!

[Alex Christensen]

Nothing more is needed here.