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

Part of #1404: Avoid flushing aggregators in lockstep #1826

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

ronawho
Copy link
Contributor

@ronawho ronawho commented Oct 5, 2022

When flushing, previously all aggregators would flush to locale 0, then 1 and so on in lockstep. This created a many-to-one bottleneck, especially at scale. This updates aggregators to start flushing to the locale they last aggregated a value for. Keeping track of the last locale should be trivially fast and getting rid of the lockstep comm behavior should improve performance of all aggregated operations, including sort, at higher scales.

I'll have more comprehensive performance results later, but on an older 240 node SGI InfiniBand machine I see a small sort (512 KiB per node) go from ~1.6s to ~0.7s and a medium sort (512 MiB per node) go from ~2.5s to ~1.8s.

Part of #1404

When flushing, previously all aggregators would flush to locale 0, then
1 and so on in lockstep. This created a many-to-one bottleneck,
especially at scale. This updates aggregators to start flushing to the
locale they last aggregated a value for. Keeping track of the last
locale should be trivially fast and getting rid of the lockstep comm
behavior should improve performance of all aggregated operations,
including sort, at higher scales.

I'll have more comprehensive performance results later, but on an older
240 node SGI InfiniBand machine I see a small sort (512 KiB per node) go
from ~1.6s to ~0.7s and a medium sort (512 MiB per node) go from ~2.5s
to ~1.8s.

Part of 1404
@ronawho ronawho changed the title Avoid flushing aggregators in lockstep Part of #1404: Avoid flushing aggregators in lockstep Oct 5, 2022
Copy link
Contributor

@Ethan-DeBandi99 Ethan-DeBandi99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@stress-tess stress-tess merged commit d60cb36 into Bears-R-Us:master Oct 6, 2022
@ronawho ronawho deleted the avoid-lockstep-agg-flush branch October 13, 2022 20:12
@ronawho ronawho mentioned this pull request Nov 1, 2022
ronawho added a commit to chapel-lang/chapel that referenced this pull request Nov 30, 2022
Avoid flushing aggregators in lockstep

[reviewed by @benharsh]

When flushing, previously all aggregators would flush to locale 0, then 1 and so on in psdueo lockstep. This created a many-to-one bottleneck, especially at scale. This updates aggregators to start flushing to the locale they last aggregated a value for. Keeping track of the last locale is trivially fast and getting rid of the lockstep comm behavior improves performance, particularly at scale.

This change was made to Arkouda's aggregators in Bears-R-Us/arkouda#1826 and this PR effectively pulls in an "upstream" change. Eventually we just want Arkouda to use Chapel's aggregators, but there have been frequent enough changes to aggregators that it's been hard to time.
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.

3 participants