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

Benchmarks: wrap the IpcSender in a mutex #121

Closed
wants to merge 1 commit into from

Conversation

@dlrobertson
Copy link
Collaborator

dlrobertson commented Nov 11, 2016

After e6ea839 IpcSenders are not Sync
and therefore the implicit capture in the closure will cause compile
time failures. If the size is greater than the result of
get_max_fragment_size wrap the IpcSender in a Mutex.

This is what I used to get benchmarks working again for my tests of #94

Fixes: #120

After e6ea839 IpcSenders are not Sync
and therefore the implicit capture in the closure will cause compile
time failures. If the size is greater than the result of
get_max_fragment_size wrap the IpcSender in a Mutex.
@emilio
Copy link
Member

emilio commented Nov 11, 2016

could we clone the sender instead? Seems like locking kind of would affect the benchmarks right?

@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Nov 11, 2016

Good question, but AFAIK no we cannot. I tried that first and didn't get anywhere. Then I noticed that wait_rx was a Mutex<OsIpcReceiver> and that is when I did the same with tx. I really hate including mutexes in benchmarks, but I couldn't see another way around this.

@antrik
Copy link
Contributor

antrik commented Nov 11, 2016

What @emilio said: the whole point of #115 is that senders should be cloned, rather than sharing references across threads... This also allows getting rid of crossbeam; just moving the cloned receivers into a "simple" thread closure instead.

(When I wrote the benchmark, I didn't have a good understanding of the interactions yet; and IIRC the unix sender wasn't using an internal Arc<> yet, to make the cloning cheap... I did have a variant using an explicit Arc<> around sender, though -- but at the time, sharing borrowed references through crossbeam seemed more elegant to me...)

@danlrobertson if you are not sure how to do this, I can whip up a PR myself.

@antrik
Copy link
Contributor

antrik commented Nov 11, 2016

BTW, depending on the platform, cloning the Arc<> might actually not be cheaper than locking a mutex... Either is probably negligible though against the cost of spawning a thread.

@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Nov 11, 2016

Yup. Feel free to close this PR if you already have an implementation of this. I just wanted to post what I had to get benchmarks working again

@antrik
Copy link
Contributor

antrik commented Nov 12, 2016

Here it is: #122

I actually made two patches: the first one is a minimal fix (hint: the trick is in the mysterious let tx = tx line -- this is necessary to enforce moving the binding into the closure, i.e. capturing by value, rather than capturing a reference); while the second does away with crossbeam.

@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Nov 12, 2016

superseded by #122

@dlrobertson dlrobertson deleted the dlrobertson:fix_bench_tests branch Nov 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.