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

Don't let partial replication unnecessarily divide push commands #229

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

psalz
Copy link
Member

@psalz psalz commented Dec 4, 2023

This was surfaced by 2D splits, but can also be constructed (somewhat awkwardly) for 1D: The extra region map lookup into buffer_state.replicated_regions could sometimes cause a box with the same last writer command to be unnecessarily split into several push commands. The fix is to collect all non-replicated boxes and do a final merge before generating the push commands.

@psalz psalz requested review from PeterTh, fknorr and GagaLP December 4, 2023 16:14
Copy link

github-actions bot commented Dec 4, 2023

Check-perf-impact results: (f289611903bd51c6db3f757a138e69d1)

⚠️ Significant slowdown (>1.25x) in some microbenchmark results: building command graphs in a dedicated scheduler thread for N nodes - 1 > immediate submission to a scheduler thread / expanding tree topology

Relative execution time per category: (mean of relative medians)

  • command-graph : 0.99x
  • graph-nodes : 1.00x
  • grid : 1.00x
  • scheduler : 1.03x
  • system : 1.00x
  • task-graph : 0.99x

@psalz psalz added this to the 0.5.0 milestone Dec 4, 2023
Copy link
Contributor

@PeterTh PeterTh left a comment

Choose a reason for hiding this comment

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

LGTM!
I'm taking your word for the fact that the new unit test replicates the previously problematic behaviour.

Copy link
Contributor

@fknorr fknorr left a comment

Choose a reason for hiding this comment

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

LGTM! This now also generates the correct number of sends in the IDAG.

src/distributed_graph_generator.cc Outdated Show resolved Hide resolved
fknorr pushed a commit to fknorr/celerity-runtime that referenced this pull request Dec 5, 2023
@psalz psalz force-pushed the fix-replication-push-division branch from a8f4fe0 to d64b10e Compare December 5, 2023 09:41
@psalz psalz force-pushed the fix-replication-push-division branch from d64b10e to 5cb9dcf Compare December 5, 2023 10:06
Copy link

github-actions bot commented Dec 5, 2023

Check-perf-impact results: (4c65f1399a47e0eb1340f63004745b17)

🚀 Significant speedup (<0.80x) in some microbenchmark results: building command graphs in a dedicated scheduler thread for N nodes - 1 > immediate submission to a scheduler thread / expanding tree topology

Relative execution time per category: (mean of relative medians)

  • command-graph : 1.02x
  • graph-nodes : 1.03x
  • grid : 1.00x
  • scheduler : 0.99x
  • system : 1.01x
  • task-graph : 0.98x

@psalz psalz merged commit 7196e25 into master Dec 5, 2023
28 checks passed
@fknorr fknorr deleted the fix-replication-push-division branch December 5, 2023 11:48
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