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

Per PE Quiet interface #316

Merged
merged 9 commits into from
Jul 1, 2022
Merged

Conversation

manjugv
Copy link
Collaborator

@manjugv manjugv commented Nov 24, 2019

Closes #317

@manjugv manjugv mentioned this pull request Nov 24, 2019
@manjugv manjugv changed the title Quiet per PE interface Per PE Quiet interface Nov 24, 2019
@agrippa
Copy link
Collaborator

agrippa commented Nov 24, 2019

As a non-implementer, I may need some help with this.

Is there a performance benefit to this routine over maintaining 1 context per PE I communicate with? At the application level that might undesirably lead to memory consumption scaling with # of PEs, so are there tricks in the runtime layer that mean it won't?

Could the options parameter to shmem_ctx_create be used to set a property on contexts (e.g. SHMEM_ONLY_USED_WITH_ONE_PE) that could have a similar effect rather than introducing a new API?

@manjugv
Copy link
Collaborator Author

manjugv commented Nov 25, 2019

@agrippa That is definitely one way to do it. As a matter of fact, with that option, one can do more optimizations. However, it is much stronger semantic than the proposal here. The context is creating a communication stream. With your proposed change, you are creating a stream per PE and you have an ability to couple that stream with a thread.

@shamisp
Copy link
Contributor

shamisp commented Nov 25, 2019

@manjugv UCX support this, but if you look at the code it comes with the cost. It is not huge but you have to track outstanding messages on the PE. If this semantics is useful for OpenSHMEM users, I'm all for it.

content/shmem_pe_quiet.tex Outdated Show resolved Hide resolved
content/shmem_pe_quiet.tex Outdated Show resolved Hide resolved
@nspark
Copy link
Contributor

nspark commented Jan 10, 2022

WG feedback: Clarify that the target_pes pointer isn't accessed if npes is zero.

content/shmem_pe_quiet.tex Outdated Show resolved Hide resolved
@manjugv
Copy link
Collaborator Author

manjugv commented Feb 9, 2022

@jdinan Do you need anything else on this before merging?

content/backmatter.tex Outdated Show resolved Hide resolved
content/shmem_pe_quiet.tex Outdated Show resolved Hide resolved
@jdinan
Copy link
Collaborator

jdinan commented Feb 13, 2022

@manjugv I added a few doc editor updates for you to review and apply. I also see that there are unresolved comments on this PR. Please resolve these, e.g. by adding doc edit changes to this PR, filing new issues, new PRs, etc.

manjugv and others added 2 commits April 1, 2022 09:09
@manjugv
Copy link
Collaborator Author

manjugv commented Apr 1, 2022

@jdinan I accepted the commit. Thanks.

@jdinan
Copy link
Collaborator

jdinan commented Apr 4, 2022

@manjugv Could you please resolve the open comments on this ticket before merging? It looks like some of these should have issues created for follow-up before the next spec release.

@manjugv
Copy link
Collaborator Author

manjugv commented Jul 1, 2022

@jdinan I have resolved all comments. Thanks.

@jdinan jdinan merged commit 55f2ad5 into openshmem-org:master Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Per PE Quiet
5 participants