Bug 156771 - Stop using OSAtomicIncrement32Barrier
Summary: Stop using OSAtomicIncrement32Barrier
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-19 17:21 PDT by Alex Christensen
Modified: 2022-10-10 15:16 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.06 KB, patch)
2016-04-19 17:22 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (2.08 KB, patch)
2016-04-19 17:25 PDT, Alex Christensen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-04-19 17:21:50 PDT
Stop using OSAtomicIncrement32Barrier
Comment 1 Alex Christensen 2016-04-19 17:22:01 PDT
Created attachment 276781 [details]
Patch
Comment 2 WebKit Commit Bot 2016-04-19 17:23:51 PDT
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.
Comment 3 Alex Christensen 2016-04-19 17:25:14 PDT
Created attachment 276782 [details]
Patch
Comment 4 Darin Adler 2016-04-19 17:37:04 PDT
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;
Comment 5 Darin Adler 2016-04-19 17:45:26 PDT
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
Comment 6 Ahmad Saleem 2022-10-10 14:33:10 PDT
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!
Comment 7 Alex Christensen 2022-10-10 15:16:37 PDT
Nothing more is needed here.