Skip to content

Replace crossbeam-channel by flume#23

Draft
zazabe wants to merge 2 commits into
slog-rs:masterfrom
zazabe:flume-channel
Draft

Replace crossbeam-channel by flume#23
zazabe wants to merge 2 commits into
slog-rs:masterfrom
zazabe:flume-channel

Conversation

@zazabe

@zazabe zazabe commented Dec 31, 2020

Copy link
Copy Markdown

Replace crossbeam-channel by flume, which is coming with some advantages, see https://github.com/zesterer/flume#why-flume.

@zazabe zazabe changed the title Replace crossbeam-channel by flume Draft: Replace crossbeam-channel by flume Dec 31, 2020
@zazabe zazabe marked this pull request as draft December 31, 2020 14:36
@zazabe zazabe changed the title Draft: Replace crossbeam-channel by flume Replace crossbeam-channel by flume Dec 31, 2020
@zazabe

zazabe commented Dec 31, 2020

Copy link
Copy Markdown
Author

not sure if i'm right, but it seems flume::Sender/Reciever are not unwind-safe, because they are dependent on Spinlock, with no lock poisoning.

So it doesn't work well with slog::SendSyncUnwindSafeDrain trait...

@vorner

vorner commented Dec 31, 2020

Copy link
Copy Markdown
Collaborator

Aren't spin locks kind of a bad idea in general?

@zazabe

zazabe commented Dec 31, 2020

Copy link
Copy Markdown
Author

Not sure, maybe they have good reasons to use spinlocks ...

I created this PR because we found some performance issues with slog::async + crossbeam-channel.
For our use-case, with multiple log producers, channel send long-tail performance can be quite bad, so we decided to downgrade slog-async to 0.2.3 for now, which uses std::mpsc.
According to our benchmarks, flume doesn't have this perf degradation.

But it's probably more a crossbeam issue, i'll open a ticket there.

@vorner

vorner commented Dec 31, 2020

Copy link
Copy Markdown
Collaborator

Not sure, maybe they have good reasons to use spinlocks ...

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.

@dpc

dpc commented Jan 1, 2021

Copy link
Copy Markdown
Contributor

Well... I'm not sure what to do about it. :D

@vorner

vorner commented Jan 3, 2021

Copy link
Copy Markdown
Collaborator

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.

@dpc

dpc commented Jan 4, 2021

Copy link
Copy Markdown
Contributor

So... all agree that flume is better? :D

@Techcable

Copy link
Copy Markdown
Member

Is this still a draft?

@dpc

dpc commented Feb 10, 2022

Copy link
Copy Markdown
Contributor

Could be a config (feature) option.

@njam

njam commented Feb 11, 2022

Copy link
Copy Markdown
Collaborator

Just to add that @zazabe and I eventually settled on using std::sync::mpsc and not flume because the performance was better with mpsc for our use case.

See some benchmark data here: #21 (comment)

@Techcable

Techcable commented Feb 11, 2022

Copy link
Copy Markdown
Member

Maybe we should put both flume and crossbeam behind feature flags, that way people can pick whatever they want.....

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.

5 participants