Skip to content

address comment#325

Merged
liunyl merged 1 commit into
mainfrom
index_mem
Dec 26, 2025
Merged

address comment#325
liunyl merged 1 commit into
mainfrom
index_mem

Conversation

@liunyl

@liunyl liunyl commented Dec 26, 2025

Copy link
Copy Markdown
Contributor

Here are some reminders before you submit the pull request

  • Add tests for the change
  • Document changes
  • Reference the link of issue using fixes eloqdb/tx_service#issue_id
  • Reference the link of RFC if exists
  • Pass ./mtr --suite=mono_main,mono_multi,mono_basic

Summary by CodeRabbit

  • Refactor
    • Optimized internal memory accounting for data synchronization operations with improved precision in buffer allocation calculations across different system configurations.

✏️ Tip: You can customize this high-level summary in your review settings.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ubuntu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Dec 26, 2025

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

Walkthrough

This pull request refactors memory accounting for flush data size in local_cc_shards.cpp by introducing conditional compilation logic. When WITH_JEMALLOC is defined, it uses sizeof(FlushRecord) for accounting; otherwise, it uses mi_malloc_usable_size() on vector buffers to measure actual allocated memory for DataSyncVec, ArchiveVec, and MoveBaseIdxVec.

Changes

Cohort / File(s) Summary
Memory accounting refactor
tx_service/src/cc/local_cc_shards.cpp
Introduces conditional memory sizing based on WITH_JEMALLOC flag. When jemalloc is enabled, uses fixed sizeof(FlushRecord) for all three vectors (DataSyncVec, ArchiveVec, MoveBaseIdxVec). When disabled, measures actual usable buffer sizes via mi_malloc_usable_size(). Applied consistently in both DataSyncForRangePartition and DataSyncForHashPartition code sections.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • #323: Modifies the same file and memory accounting logic for FlushRecord/vector buffers, exploring sizeof versus malloc_usable_size trade-offs.

Suggested reviewers

  • xiexiaoy
  • MrGuin

Poem

🐰 A rabbit hops through memory lanes so bright,
Choosing paths with jemalloc's might,
Or mimalloc's malloc_usable_size—
Each buffer measured, memory precise,
Three vectors dance in perfect sync,
No more guessing, just the facts we think! 🎪

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch index_mem

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c5f9174 and 2e21758.

📒 Files selected for processing (1)
  • tx_service/src/cc/local_cc_shards.cpp

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 and usage tips.

@liunyl liunyl merged commit a2d73fb into main Dec 26, 2025
2 of 4 checks passed
@liunyl liunyl deleted the index_mem branch December 26, 2025 07:29
@coderabbitai coderabbitai Bot mentioned this pull request Jan 14, 2026
@coderabbitai coderabbitai Bot mentioned this pull request Jan 30, 2026
5 tasks
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