-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add ftl::Fibtex #66
Add ftl::Fibtex #66
Conversation
include/ftl/atomic_counter.h
Outdated
/** | ||
* A wrapper over std::atomic_uint::compare_exchange_strong() | ||
* | ||
* The compare_exchange_strong *with* be atomic, but this function as a while is *not* atomic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pedantic, but with should be will
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also while should be whole
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha good catch, my brain isn't in gear today 😄
include/ftl/atomic_counter.h
Outdated
* | ||
* \param expectedValue The value that is expected to be in the atomic counter | ||
* \param newValue The value that the atomic counter will be set to if comparison succeeds. | ||
* \param memoryOrder The memory order to use for the compare_exchange_strong |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thing, but if you're in there fixing the above, can you have 4 spaces after the longest parameter value. Also, can you updated it to @param instead of \param. So the style matches the other comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 That's what my editor generates (as I use that style) but I obviously didn't catch them all.
include/ftl/fibtex.h
Outdated
/** | ||
* All Fibtex's have to be aware of the task scheduler in order to yield. | ||
* | ||
* @param taskScheduler ftl::TaskScheduler that will be using this mutex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, can it be:
* @param taskScheduler ftl::TaskScheduler that will be using this mutex.
Once you're done with the code review, I'll commit all the changes. |
include/ftl/fibtex.h
Outdated
|
||
namespace ftl { | ||
/** | ||
* A fiber aware mutex. Does not block in the traditional way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a note here that the member function names are named to so they can be used with C++ standard stuff?
Overall, the code changes are great. Just small comments / documentation stuff for fixes. I really like the |
include/ftl/fibtex.h
Outdated
} | ||
|
||
/** | ||
* Lock mutex using an finite spinlock. Does not spin if there is only one backing thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using a finite
include/ftl/config.h
Outdated
@@ -53,6 +53,13 @@ | |||
#endif | |||
#endif | |||
|
|||
#ifdef __SSE2__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://msdn.microsoft.com/en-us/library/b0084kay.aspx
MSVC does not set this macro, even when compiling with SSE2 instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof. I'm basically detecting for <emmintrin.h>
and _mm_pause()
. I'll see if there's a better way of doing this.
Hey, looks great so far! Fibtex is missing thread pinning support, which is unfortunately something that is needed for adoption. |
include/ftl/fibtex.h
Outdated
*/ | ||
void unlock() { | ||
if (!m_atomicCounter.CompareExchange(1, 0)) { | ||
printf("Mutex was unlocked by another fiber or was double unlocked.\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this again, this should probably be more shouty. At least an assert(), and maaaaybe an exception. C++ standard doesn't have any definition for double unlocking. It just says it's undefined behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would take assert. There is a big programming error like that it really should die on debug mode. I'll add that.
@martty Thanks! How would you expect this to be implemented? Should the mutex always resume on the same thread, or should it just have an option with the lock functions to be able to do that (defaulted parameter). |
I'm also going to add some RAII helpers for things |
Following the API of WaitForCounter, I think a defaulted ctor argument would be fine (default = unpinned). |
Can you explain why you chose to implement three |
At the bare minimum I wanted to implement a yielding lock and a spin lock. I split up the two spin lock functions because the behavior of |
Having multiple |
@martty This is why I am currently working on specific lock guards for use with these functions. I'll push them momentarily for comment. |
I want to avoid making the mutex itself spin/yield specific, as you might want to have it spin in some situations but yield in others. |
Sure, I meant specifying the spinning mechanism with a constructor parameter or a template parameter. |
That's what I mean, sorry if I wasn't clear. I don't want to force a particular instance of a lock to be bound to a locking type because you may need to use the same lock in different ways depending on the situation. If fiber 0 is just updating a flag guarded by the lock, but fiber 1 is updating the whole data structure, Fiber 0 will want to do a blocking lock, and fiber 1 will want to do a spin lock. |
I have implemented lock guards and I'll implement unqiue locks in a bit, but I need to go home, so I put these up for review in the mean time 😄 |
Nice job! |
Alright, I've added a basic Unique Lock impl. I have followed the standard (i.e. cppreference) to the letter with the exception of the swap function being a friend function to replace the the specialization of std::swap (which isn't possible). If you think we should deviate from the standard at any point lmk. I'll get on documenting and building the 2 other unique locks. |
Can you make FibTex noncopyable and nonmovable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make these: GetThreadCount()
and GetFiberCount()
as const
@ChemiaAion I'll do a pass over all of the members and make them as restrictive as possible. |
Still need to do a round of commenting and such, but the functionally should all be there at this point unless someone thinks of something else that should be there. |
This PR should be ready to go in terms of functionality. Github is still a little hiccup-ey and we still have that random segfault bug in the tests. I might also add some tests to make sure it clears all the compilers. |
The rationale behind changing the names of the spin locks is that having an infinite spin is almost never the right thing to do. This change makes the right-er thing a shorter name, and makes it clear which will put you in a spin lock infinitely.
The biggest thing that seems to be blocking ftl::Fibtex from merging is this race condition that is described in #75. |
I am going to close this to prevent it from cluttering things up because I no longer have the time to work on it, and I have basically abandoned c++ for my projects. Keep up the good work y'all. |
Here's the implementation of a mutex for fibers. Closes #64.