-
Notifications
You must be signed in to change notification settings - Fork 24
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
Replace crossbeam-channel by flume #23
base: master
Are you sure you want to change the base?
Conversation
not sure if i'm right, but it seems So it doesn't work well with |
Aren't spin locks kind of a bad idea in general? |
Not sure, maybe they have good reasons to use spinlocks ... I created this PR because we found some performance issues with But it's probably more a crossbeam issue, i'll open a ticket there. |
They are generally quite fast because they are simple and they don't need OS support. But if the thread is ever preempted while the lock is held, other threads will burn CPU for the whole scheduler tick (and even yielding to the OS scheduler doesn't have to help, if enough of them are spinning, they will keep turns). So in a sense, they can be faster, but they also have much more severe „failure“ mode. That's why eg. parking_lot spins for few turns, but then suspends on a real OS-backed mutex. I guess the problem with crossbeam is that it runs kind of GC thing from time to time, which takes time on one of the CPUs. But I'd be wary of putting a spinlock into logging for the above reason. |
Well... I'm not sure what to do about it. :D |
OK, I've had a look at flume and it seems not to be using the spin lock as a mutex: zesterer/flume#29 (comment). So my fear of spin-locks here is probably not warranted. |
So... all agree that |
Is this still a draft? |
Could be a config (feature) option. |
Just to add that @zazabe and I eventually settled on using See some benchmark data here: #21 (comment) |
Maybe we should put both flume and crossbeam behind feature flags, that way people can pick whatever they want..... |
Replace
crossbeam-channel
by flume, which is coming with some advantages, see https://github.com/zesterer/flume#why-flume.