Skip to content

misc: release old buffers before ensureCapacity growth - #804

Open
lizhen-0710 wants to merge 1 commit into
bytedance:mainfrom
lizhen-0710:misc-ensure-capacity-release-buffer
Open

misc: release old buffers before ensureCapacity growth#804
lizhen-0710 wants to merge 1 commit into
bytedance:mainfrom
lizhen-0710:misc-ensure-capacity-release-buffer

Conversation

@lizhen-0710

@lizhen-0710 lizhen-0710 commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Release the existing buffer before allocating a larger replacement in dwio::common::ensureCapacity(). The helper is used by Parquet decode buffers and other scan-side scratch buffers where growth can otherwise transiently hold both the old large allocation and the new larger allocation in the same memory pool.

Type of Change

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 🚀 Performance improvement (optimization)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🔨 Refactoring (no logic changes)
  • 🔧 Build/CI or Infrastructure changes
  • 📝 Documentation only

Description

This reduces short-lived scan memory spikes without changing the returned buffer contents contract: callers already request capacity growth and do not depend on preserving old contents through this helper.

Add a regression test with a constrained memory pool. The test derives the old and new capacities from the allocator preferred size, then requests one byte beyond the old capacity so ensureCapacity must reallocate. Without releasing the old buffer first, the constrained pool cannot hold the old and new allocations at the same time and the test fails with MEM_CAP_EXCEEDED.

Performance Impact

  • No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).

  • Positive Impact: I have run benchmarks.

    Click to view Benchmark Results
    Paste your google-benchmark or TPC-H results here.
    Before: 10.5s
    After:   8.2s  (+20%)
    
  • Negative Impact: Explained below (e.g., trade-off for correctness).

Release Note

Please describe the changes in this PR

Release Note:

Release Note:
- Fixed a crash in `substr` when input is null.
- optimized `group by` performance by 20%.

Checklist (For Author)

  • I have added/updated unit tests (ctest).
  • I have verified the code with local build (Release/Debug).
  • I have run clang-format / linters.
  • (Optional) I have run Sanitizers (ASAN/TSAN) locally for complex C++ changes.
  • No need to test or manual test.

Breaking Changes

  • No

  • Yes (Description: ...)

    Click to view Breaking Changes
    Breaking Changes:
    - Description of the breaking change.
    - Possible solutions or workarounds.
    - Any other relevant information.
    

auto rootPool =
memoryManager()->addRootPool("ensureCapacityReleaseBeforeAlloc", 6 << 20);
auto leafPool = rootPool->addLeafChild("leaf");
BufferPtr buffer = AlignedBuffer::allocate<char>(4 << 20, leafPool.get());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AlignedBuffer::allocate allocates more than 4M due to kPaddingSize, roughly equals to 1.5*4M. So ensureCapacity may not trigger date.reset and reallocate

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, updated, thanks.

@lizhen-0710
lizhen-0710 force-pushed the misc-ensure-capacity-release-buffer branch from 45291d2 to a3056a3 Compare July 29, 2026 13:47
@lizhen-0710
lizhen-0710 requested a review from guhaiyan0221 July 29, 2026 13:47
@lizhen-0710
lizhen-0710 force-pushed the misc-ensure-capacity-release-buffer branch from a3056a3 to 7b91c27 Compare July 29, 2026 14:09

@guhaiyan0221 guhaiyan0221 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

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