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

Fixed unnecessary Pool creation in searchlight #386

Merged
merged 3 commits into from Oct 16, 2018

Conversation

@davidt0x
Copy link
Contributor

@davidt0x davidt0x commented Oct 15, 2018

When pool_size=1 the creation of a multiprocessing pool is unnecessary
and wasteful because data needs to be copied and sent to other process.
This would double the needed memory for each MPI task. In addition,
fork() can cause unpredictable behaviour in some MPI implementations,
see:

https://www.open-mpi.org/faq/?category=tuning#fork-warning

When pool_size=1 the creation of a multiprocessing pool is unnecessary
and wasteful because data needs to be copied and sent to other process.
This would double the needed memory for each MPI task. In addition,
fork() can cause unpredictable behaviour in some MPI implementations,
see:

https://www.open-mpi.org/faq/?category=tuning#fork-warning
@buildbot-princeton
Copy link
Collaborator

@buildbot-princeton buildbot-princeton commented Oct 15, 2018

Can one of the admins verify this patch?

Loading

1 similar comment
@buildbot-princeton
Copy link
Collaborator

@buildbot-princeton buildbot-princeton commented Oct 15, 2018

Can one of the admins verify this patch?

Loading

@mihaic
Copy link
Contributor

@mihaic mihaic commented Oct 15, 2018

Jenkins, add to whitelist.

Loading

Copy link
Contributor

@mihaic mihaic left a comment

@davidt0x, thanks for the PR. It makes sense. Could you please fix the lint errors?

Loading

@mihaic
Copy link
Contributor

@mihaic mihaic commented Oct 16, 2018

Jenkins, retest this please.

Loading

@davidt0x
Copy link
Contributor Author

@davidt0x davidt0x commented Oct 16, 2018

Do we need a unit test for pool_size = 1 to get coverage up? Am I reading this correctly?

Loading

@mihaic
Copy link
Contributor

@mihaic mihaic commented Oct 16, 2018

Precisely!

Loading

@davidt0x
Copy link
Contributor Author

@davidt0x davidt0x commented Oct 16, 2018

Ok, I copied one of the previous tests with the only difference being pool_size=1. Test passes and coverage was good on my end. Third times the charm hopefully :)

Loading

mihaic
mihaic approved these changes Oct 16, 2018
@mihaic mihaic merged commit 8994231 into brainiak:master Oct 16, 2018
5 checks passed
Loading
@mihaic
Copy link
Contributor

@mihaic mihaic commented Oct 16, 2018

Thanks a lot, @davidt0x! This should make some people happy. :)

Loading

@davidt0x davidt0x deleted the searchlight_poolsize_1_opt branch Oct 17, 2018
mihaic added a commit to mihaic/brainiak that referenced this issue Jun 21, 2019
mihaic added a commit to mihaic/brainiak that referenced this issue Jun 21, 2019
mihaic added a commit that referenced this issue Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants