Skip to content
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.

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

Atomic for BsModule #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonesmz
Copy link
Contributor

@jonesmz jonesmz commented Mar 31, 2018

No description provided.

@BearishSun
Copy link
Member

Earlier on I decided against atomicity for Module. Primarily because it is something you end up paying for on every single call to Module::instance(), and it only really matters during startUp/shutDown calls, which happen once at very specific points.

The memory barriers will prevent the compiler from optimally organizing the code around the relevant call, and can potentially issue a memory barrier instruction (will happen on ARM I believe, but on x86 just guaranteeing the order is enough). Perhaps if the atomicity could be achieved using relaxed memory ordering it would be acceptable but I'm not sure I'd like it even then though.

If we were to go a thread safe route I'd prefer we use a separate version of Module that is thread-safe (perhaps via a policy controlled by a template parameter).

@jonesmz
Copy link
Contributor Author

jonesmz commented Mar 31, 2018

Ok, that sounds good.

I'll add a set of bs::StoragePolicy classes that can be given as a template parameter.

Normal, which would just be the normal type being stored.
Thread local, which would be for thread local storage
Atomic, which would wrap the type in std::atomic.

@jonesmz jonesmz force-pushed the atomic_for_bs_module branch from 16c9356 to c6cdcbd Compare March 31, 2018 23:50
@jonesmz jonesmz force-pushed the atomic_for_bs_module branch 4 times, most recently from 5ee20b2 to b199601 Compare April 10, 2018 18:53
@jonesmz jonesmz force-pushed the atomic_for_bs_module branch 2 times, most recently from 102ce6d to e501e7b Compare May 13, 2018 06:56
@jonesmz jonesmz force-pushed the atomic_for_bs_module branch 2 times, most recently from 236daeb to aeb6297 Compare May 16, 2018 20:00
@jonesmz
Copy link
Contributor Author

jonesmz commented Jul 9, 2018

Sorry for the long delay on this issue. Busy with other stuff shrug. Still planning to provide a patch as described.

@jonesmz jonesmz force-pushed the atomic_for_bs_module branch from aeb6297 to eb38f14 Compare July 11, 2018 04:55
@jonesmz jonesmz force-pushed the atomic_for_bs_module branch 2 times, most recently from 198a307 to dc167f9 Compare October 6, 2019 19:29
@jonesmz jonesmz force-pushed the atomic_for_bs_module branch from dc167f9 to a5fd96e Compare October 6, 2019 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants