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

Adjust CopyAggregation aggregators to have the option of not freeing buffers on flush #26681

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mppf
Copy link
Member

@mppf mppf commented Feb 7, 2025

Before this PR, CopyAggregation aggregators always free the buffers on a flush, because that function was written primarily for the case of freeing the aggregator, in which case freeing the buffers immediately avoids the need to do it later.

However, in some application use cases, it's useful to keep aggregators around for longer to avoid overheads of creating and destroying aggregators. It might be necessary to call 'flush' on SrcAggregator so that the appropriate data is loaded, but that aggregator might be used again, so it's not helpful to free the buffers.

This PR adjusts 'flush' to accept an argument to indicate if the buffers should be freed or not. In my opinion, it would make more sense for the default for flush to be to not free the buffer. However, this PR does not change that, to avoid creating performance noise for any existing uses of this function (as in Arkouda).

This change was suggested by @ronawho.

  • full comm=none testing
  • full comm=gasnet oversubscribed testing

mppf added 4 commits February 7, 2025 17:05
---
Signed-off-by: Michael Ferguson <[email protected]>
---
Signed-off-by: Michael Ferguson <[email protected]>
@ronawho
Copy link
Contributor

ronawho commented Feb 8, 2025

I think the original use case when the initial API was done was an SPMD region doing something like:

// Collect remote data
var srcAgg = new SrcAgg();
for i in 1..n do srcAgg.copy(...);
srcAgg.flush();

// permute the local data in some way

// Put local data back to remote (maybe it was in transposed order)
var dstAgg = new DstAgg();
for i in 1..n do dstAgg.copy(...);
dstAgg.flush();

So we needed to flush the aggregator before it would naturally fall out of scope, but never used it again. I didn't want to add an artificial scope just to deinit the aggregator and the combined flush+free-remote-buffers eliminates any comm when it finally does go out of scope.

I agree there are use-cases where you want it either way so adding an option makes sense to me. Most of the time aggregators get used as a task-private variable in a forall and without an explicit flush, so I don't think it's a very common idiom to manually flush anyways. Given that I don't think you need to worry too much about which default is better and I would also just default to the current behavior (but if designing from scratch would probably default to false or have a different operation that does the flush+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.

2 participants