8000 mem: Make nrefs atomic by Lastique · Pull Request #446 · baresip/re · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

mem: Make nrefs atomic #446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 21, 2022
Merged

mem: Make nrefs atomic #446

merged 4 commits into from
Jul 21, 2022

Conversation

Lastique
Copy link
Contributor

This allows to use mem API for shared memory objects from multiple threads.

@sreimers
Copy link
Member
sreimers commented Jul 17, 2022 •
edited
Loading

I can add C99 (mutex) and WIN32 fallbacks for atomic_load_explicit and atomic_store_explicit... within the next days.

@Lastique
Copy link
Contributor Author

I'm going to expand re_atomic.h to support compiler intrinsics. Then this should compile in C99 mode.

@Lastique Lastique force-pushed the feature/mem_atomic_nrefs branch 2 times, most recently from 00636d2 to a167c3c Compare July 17, 2022 16:39
sreimers
sreimers reviewed Jul 18, 2022
View reviewed changes
sreimers
sreimers reviewed Jul 18, 2022
View reviewed changes
@sreimers
Copy link
Member

I'm going to expand re_atomic.h to support compiler intrinsics. Then this should compile in C99 mode.

I think full compiler intrinsics fallback is not needed (since we want to drop C99 maybe shortly, we should keep this simple) and add only needed fallbacks. ffmpeg replaces this with pthread mutex. For fast and full atomic support a C11 ready compiler should be used.

https://github.com/FFmpeg/FFmpeg/blob/811f2f91da101db02ee8b1f0e4f6276e5aecda5e/compat/atomics/pthread/stdatomic.h#L98-L111

@Lastique
Copy link
Contributor Author

I don't think MSVC implements C11. And C99 is still required.

Also, I do not like using a mutex to implement atomics, this is way too much overhead.

@sreimers
Copy link
Member

Ok but I think we can drop the __sync intrinsics, __atomic* intrinsics. should be enough since gcc supports these since 4.7 right? We only support gcc 4.9 or later.

And I would prefer to keep the naming clean like: re_atomic_store and if needed we can add re_atomic_store64 later.

@Lastique Lastique force-pushed the feature/mem_atomic_nrefs branch 4 times, most recently from f364630 to 1762db8 Compare July 19, 2022 17:27
@Lastique
Copy link
Contributor Author

I think we can drop the __sync intrinsics

Ok.

And I would prefer to keep the naming clean like: re_atomic_store and if needed we can add re_atomic_store64 later.

I have made the macros size-agnostic, although it complicated things for MSVC quite a bit. Hopefully, it should be able to optimize away all that cruft.

@Lastique Lastique force-pushed the feature/mem_atomic_nrefs branch from 1762db8 to 0a639fb Compare July 19, 2022 17:36
@Lastique Lastique requested a review from sreimers July 19, 2022 17:41
@Lastique Lastique force-pushed the feature/mem_atomic_nrefs branch 2 times, most recently from 8fdf1ee to 2e49aff Compare July 19, 2022 18:03
sreimers
sreimers reviewed Jul 19, 2022
View reviewed changes
sreimers
sreimers reviewed Jul 19, 2022
View reviewed changes
< 8000 div data-view-component="true">
Lastique added 4 commits July 20, 2022 11:10
@Lastique
atomic: Implemented abstraction layer similar to C11 atomics.
399c1c5 
This new abstraction layer defines a set of macros that resemble C11
atomics interface and are implemented in terms of C11 atomics or compiler
intrinsics, depending on the compiler.

Besides C11 atomics, gcc __atomic, and MSVC Interlocked intrinsics are
supported. MSVC implementation has a caveat that it has to convert its
arguments and returned values to the largest supported integer size,
unsigned __int64. This may cause warnings in the calling code where
this results in implicit conversions. These warnings can be worked around
with explicit casts.
@Lastique
mem: Make nrefs atomic.
1cd8d92 
This allows to use mem API for shared memory objects from multiple threads.
@Lastique
main: Updated the use of atomics to use explicit atomic functions.
5b80b32 
This ensures that the atomic variables are actually used as atomics on
compilers that don't support C11 _Atomic keyword. Also, this changes
the memory ordering to relaxed on all accesses, as no memory ordering
is necessary in all cases.
@Lastique
Revert "cmake: add explicit MSVC /volatile:ms (required for arm) (bar…
1df080c 
…esip#451)"

This reverts commit 1e397ae.

Reason for revert: This is no longer needed as we are using intrinsics
for atomics now.
@Lastique Lastique force-pushed the feature/mem_atomic_nrefs branch from 2e49aff to 1df080c Compare July 20, 2022 08:11
@Lastique Lastique requested a review from sreimers July 20, 2022 08:11
@sreimers
Copy link
Member

Great work, thanks!

@sreimers sreimers merged commit 50f7df9 into baresip:main Jul 21, 2022
@Lastique Lastique deleted the feature/mem_atomic_nrefs branch July 21, 2022 08:40
@cspiel1
Copy link
Collaborator
cspiel1 commented Aug 3, 2022

I am afraid, but we need C99 support up to the next 3-5 Years. So we need an abstraction for C99. I would prefer in mainline. Maybe we could separate it to an extra header file if this helps.

So far I have no idea how this could be implemented. Maybe with pthread mutexes?

@sreimers
Copy link
Member
sreimers commented Aug 3, 2022 •
edited
Loading

Have you build issues? We have already gcc-style __atomic* intrinsics in re_atomic.h

@cspiel1
Copy link
Collaborator
cspiel1 commented Aug 3, 2022

Yes, indeed for our old product we get:

include/re_atomic.h:711:2: error: #error "Compiler does not support atomics"

We have bfin-linux-uclibc-gcc-4.3.5. And no chance for an update. If nothing helps, we will try to make a legacy patch. But of course we would prefer to get rid of all our private patches.

@Lastique
Copy link
Contributor Author
Lastique commented Aug 3, 2022 •
edited
Loading

gcc 4.3 supports __sync_* intrinsics. Those won't be as efficient as __atomic intrinsics, but definitely better than a mutex. @sreimers Would you like to revive them in re_atomic.h?

@sreimers
Copy link
Member
sreimers commented Aug 3, 2022

Yes lets do that.

@Lastique Lastique mentioned this pull request Aug 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sreimers sreimers Awaiting requested review from sreimers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Lastique @sreimers @cspiel1
0