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

RemoteRefs refer to any type of AbstractChannel #12385

Merged
merged 1 commit into from
Aug 3, 2015

Conversation

amitmurthy
Copy link
Contributor

With this PR:

  • RemoteRefs refer to an AbstractChannel
  • A concrete implementation of AbstractChannel, implements take!, fetch, put!, isready and wait
  • New constructor RemoteRef(f::Function, pid=myid()) where f() must return an AbstractChannel, is used to construct a remote channel on worker pid.
  • The current RemoteRef constructors create a remote Channel{Any}(1) which mimics the current behavior.

The larger and deeper issue of redoing distributed gc in #12042 can be deferred to 0.5

We don't break any existing code and this is a minimal change that enables us to have remote channels of specified type and length in 0.4 itself. And more importantly unifies RemoteRefs and Channels.

cc @JeffBezanson, @jakebolewski , @ViralBShah, @malmaud

@amitmurthy amitmurthy force-pushed the amitm/remoterefs_with_channels branch from de2d66d to 0393ae2 Compare July 31, 2015 08:37
@amitmurthy
Copy link
Contributor Author

Have added docs and tests. Please refer to the manual changes and added example for a more detailed explanation of this PR.

@ViralBShah ViralBShah added this to the 0.4.0 milestone Jul 31, 2015
@amitmurthy amitmurthy force-pushed the amitm/remoterefs_with_channels branch from 0393ae2 to aeca5a9 Compare July 31, 2015 12:21
@amitmurthy
Copy link
Contributor Author

Feedback? Bikeshedding names?

@malmaud
Copy link
Contributor

malmaud commented Aug 1, 2015

How hard would it be in this design to used shared memory for implementing remote references if the workers are all on the same node?

@amitmurthy
Copy link
Contributor Author

Well, the usual considerations in using shmem for IPC apply.

A ShmemChannel <: AbstractChannel would need to

  • map shmem segments on all workers in its constructors
  • limit itself to either dealing with bitstypes or support non-bitstypes objects in their serialized form
  • use a portable locking/signalling library for co-ordinating between processes

@amitmurthy amitmurthy force-pushed the amitm/remoterefs_with_channels branch from aeca5a9 to 02eb352 Compare August 2, 2015 03:20
@amitmurthy
Copy link
Contributor Author

Will merge in a day or two.

@amitmurthy amitmurthy force-pushed the amitm/remoterefs_with_channels branch from ef917b0 to 4d08b6d Compare August 3, 2015 04:22
@amitmurthy
Copy link
Contributor Author

Have rebased and removed changes to doc/stdlib/parallel.rst. Will merge once CI is green.

@amitmurthy
Copy link
Contributor Author

Error in test/profile.jl seems unrelated to this PR - https://gist.github.com/amitmurthy/38e4bac61adc76b7fbdb

restarting job.

amitmurthy added a commit that referenced this pull request Aug 3, 2015
RemoteRefs refer to any type of AbstractChannel
@amitmurthy amitmurthy merged commit 8dcdf83 into master Aug 3, 2015
@amitmurthy amitmurthy deleted the amitm/remoterefs_with_channels branch August 3, 2015 05:28
@tkelman
Copy link
Contributor

tkelman commented Aug 3, 2015

Have rebased and removed changes to doc/stdlib/parallel.rst. Will merge once CI is green.

I think those should be re-added, but now in helpdb

@tkelman
Copy link
Contributor

tkelman commented Aug 3, 2015

strange parallel segfault on osx travis a few builds after this was merged https://travis-ci.org/JuliaLang/julia/jobs/73898628, might be unrelated though?

@amitmurthy
Copy link
Contributor Author

Hmmm, the PR was Julia code only and seg faults usually point to a bug elsewhere - I don't know if this PR triggered some other fault elsewhere.

https://gist.github.com/amitmurthy/38e4bac61adc76b7fbdb above was also an invalid free.

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