Skip to content

perf(algo): pre-size heap in internalMergeSortWithBuffer - #9715

Merged
shiva-istari merged 1 commit into
dgraph-io:mainfrom
shaunpatterson:perf/algo-uidlist-heap
Oct 15, 2026
Merged

shiva-istari merged 1 commit into
dgraph-io:mainfrom
shaunpatterson:perf/algo-uidlist-heap

Conversation

@shaunpatterson

@shaunpatterson shaunpatterson commented May 28, 2026 •

Copy link
Copy Markdown
Contributor

Build the heap slice and call heap.Init (O(n)) instead of n×heap.Push (per-element interface boxing).

BenchmarkMergeSorted/MergeSorted (10000 lists × 100 uids): allocs/op 20.02k→10.01k (-50%), B/op 11.46Mi→8.86Mi (-22.7%).


Independent change; branched from 4b9b399d (no dependency on the other PRs in this set). Verified: package unit tests pass + Go benchmark (benchstat, p<0.01). Numbers are single-thread microbenchmarks; not yet run through the Docker integration suite.

Summary by CodeRabbit

  • Performance
    • Improved initialization efficiency when merging UID lists. Output and behavior remain unchanged.

@shaunpatterson
shaunpatterson requested a review from a team as a code owner May 28, 2026 18:18
@shaunpatterson
shaunpatterson force-pushed the perf/algo-uidlist-heap branch from 8cec1aa to 4e2d1a4 Compare July 14, 2026 01:38
@shiva-istari
shiva-istari force-pushed the perf/algo-uidlist-heap branch from 4e2d1a4 to 414d90b Compare October 15, 2026 07:41
@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 45c4468f-17b5-4490-86cd-601ece5c0100
📥 Commits

Reviewing files that changed from the base of the PR and between 414d90b and 0f6732c.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0c1422e9-f113-4b9a-afb5-dc94ed81757a
📥 Commits

Reviewing files that changed from the base of the PR and between 2c125bd and 414d90b.

📒 Files selected for processing: 1
  • algo/uidlist.go

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

📝 Walkthrough

internalMergeSortWithBuffer now preallocates heap capacity for the input lists, appends each eligible list’s first UID, and calls heap.Init once instead of calling heap.Push for each list.

Changes: UID list heap initialization

Layer / File(s) Summary
Initialize the merge heap
algo/uidlist.go
The function preallocates the heap slice, appends the first UID from each nonempty list, and initializes the heap once.

Priority: ⬇️ Low

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

Change: Refactor

Suggested reviewers: mlwelles

Merge Risk: ⚪ Minimal · up to 414d9

The heap initialization change preserves merge behavior, with no material merge risk identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: pre-sizing the heap in internalMergeSortWithBuffer for performance.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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

Comment @coderabbitai help to get the list of available commands.

Build the heap slice and call heap.Init (O(n)) instead of n×heap.Push
(per-element interface boxing).

BenchmarkMergeSorted/MergeSorted (10000 lists × 100 uids):
  allocs/op 20.02k -> 10.01k (-50.03%, p=0.002)
  B/op      11.46Mi -> 8.86Mi (-22.70%, p=0.002)
@shiva-istari
shiva-istari force-pushed the perf/algo-uidlist-heap branch from 414d90b to 0f6732c Compare October 15, 2026 09:39
@shiva-istari
shiva-istari merged commit 4e56613 into dgraph-io:main Oct 15, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants