[hkaiser]

parallel_merge passed the longer of its two input ranges to the partitioner, so the executor parameters saw between half and all of the work of the merge. For two inputs of equal length they saw only half of the iterations, so executor parameters that derive the number of cores or the chunk size from the number of iterations and the run time of one iteration based their decision on the wrong amount of work.

The chunks of the merge only use their index into the output: the diagonal index finds where each chunk starts in both inputs. The partitioner now runs over a counting range of all output elements (hpx::util::counting_iterator<std::size_t>), so that the executor parameters see the whole work of the merge. The result of the algorithm is computed from the known ends of both inputs instead of from the iterator the partitioner returns, which removes the two separate calls for the longer first or second input.

benchmark_merge_sweep computes the time per iteration it gives adaptive_chunk_size per output element accordingly.

All merge tests pass, and benchmark_merge takes the same time as before (189.5 s vs 189.9 s).

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 64df2ca6-521d-40c5-bfa1-a01c01ed21d9

Commits

Reviewing files that changed from the base of the PR and between 33594d6 and 8dac7d3.

Files selected for processing: 1

• libs/core/algorithms/include/hpx/parallel/algorithms/merge.hpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Summary

Summary by CodeRabbit

• Improvements
  • Parallel merge now distributes work across the full output range, including when the input ranges differ in size.
• Tests
  • Merge benchmark results now report sequential time per element based on the combined size of both input ranges, providing a consistent measure across different workloads.

parallel_merge partitions work across the combined output length and returns both input end iterators with the destination advanced by that length. The merge benchmark divides sequential merge time by the combined input lengths.

Changes: Merge Output Partitioning

Layer / File(s)	Summary
Partition merge work and update benchmark accounting libs/core/algorithms/include/hpx/parallel/algorithms/merge.hpp, libs/core/algorithms/tests/performance/benchmark_merge_sweep.cpp	parallel_merge uses diagonal indices to partition the combined output range and returns both input end iterators with the destination advanced by the total input length. The benchmark divides sequential merge time by the combined input lengths.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

• Let tasks carry their expected run time (hpx::task_size) for scheduling decisions #7837: The change uses the full output length for parallel_merge partitioning, which is relevant to task-size information used for scheduling decisions.

Possibly related PRs

• TheHPXProject/hpx#7948: This PR changes the iteration count passed to the merge partitioner, which feeds the chunking decisions added by #7948.
• TheHPXProject/hpx#7926: This PR changes the iteration count used by the task-size estimate added by #7926.

Suggested reviewers: v4xsh

Merge Risk: Minimal · up to 8dac7

No merge-blocking issue was identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: partitioning the merge across all output elements.
Description check		The description directly explains the changes to parallel_merge and benchmark_merge_sweep, including their purpose and reported test results.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[hkaiser]

@kollanur Could you have a look, please?

[kollanur]

@kollanur Could you have a look, please?

sure.

[kollanur]

parallel_merge passed the longer of its two input ranges to the partitioner, so the executor parameters saw between half and all of the work of the merge. For two inputs of equal length they saw only half of the iterations, so executor parameters that derive the number of cores or the chunk size from the number of iterations and the run time of one iteration based their decision on the wrong amount of work.

The chunks of the merge only use their index into the output: the diagonal index finds where each chunk starts in both inputs. The partitioner now runs over a counting range of all output elements (hpx::util::counting_iterator<std::size_t>), so that the executor parameters see the whole work of the merge. The result of the algorithm is computed from the known ends of both inputs instead of from the iterator the partitioner returns, which removes the two separate calls for the longer first or second input.

benchmark_merge_sweep computes the time per iteration it gives adaptive_chunk_size per output element accordingly.

All merge tests pass, and benchmark_merge takes the same time as before (189.5 s vs 189.9 s).

Thanks for the changes. This makes sense, especially with the task-size estimation introduced in #7926. Since get_diagonal_index(len1 + len2) was already partitioning the entire output, and the benchmark now adjusts time_per_iteration accordingly, I'm curious whether this also affects core selection or chunk sizing for any particular executor parameters, or mainly improves task-size estimation. Looks good to me.