fix: Destroy the previously created accumulator for AggregateWindow to fix memory leak by jiangjiangtian · Pull Request #15679 · facebookincubator/velox

jiangjiangtian · 2025-12-03T04:00:53Z

One example I found is as follows: if the method initializeNewGroupsInternal in FirstLastAggregateBase is called multiple times and the function is not numeric, the previously created SingleValueAccumulator object will be overwritten without calling destroy to free its memory. This leads to memory leak and eventually causes OOM. This situation will occur in Window operator when there are multiple window frames to process.

So this PR fixes the issue by calling destroy on rawSingleGroupRow_ when processing a new window frame.

netlify · 2025-12-03T04:01:16Z

✅ Deploy Preview for meta-velox ready!

Name	Link
🔨 Latest commit	`e6e7d0a`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/69ae97ddc068270008cc3fa1
😎 Deploy Preview	https://deploy-preview-15679--meta-velox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

jinchengchenghh · 2025-12-03T10:22:25Z

I find this code https://github.com/facebookincubator/velox/pull/5632/files#diff-b9ed2eb619a914241f656abe59881d169602eed734af2df443f6480d611850aaR122-R124
to call destroy directly, maybe your exception is caused by not calling the destroy directly.

Also, maybe fix that in virtual void initializeNewGroups(
char** groups,
folly::Range<const vector_size_t*> indices) {
initializeNewGroupsInternal(groups, indices);

for (auto index : indices) {
  groups[index][initializedByte_] |= initializedMask_;
}

}
This can help eliminate the memory leak issue.
Also, does this problem only occurs when isFixedSize() false?

  virtual bool isFixedSize() const {
    return true;
  }

The initializeNewGroupsInternal should only be called when the condition is satisfied.

jiangjiangtian · 2025-12-03T11:17:51Z

I find this code https://github.com/facebookincubator/velox/pull/5632/files#diff-b9ed2eb619a914241f656abe59881d169602eed734af2df443f6480d611850aaR122-R124 to call destroy directly, maybe your exception is caused by not calling the destroy directly.

Yes, now AggregateWindow will only create a new accumulator but without destroying it.

Also, maybe fix that in virtual void initializeNewGroups( char** groups, folly::Range<const vector_size_t*> indices) { initializeNewGroupsInternal(groups, indices);
for (auto index : indices) {
  groups[index][initializedByte_] |= initializedMask_;
}
} This can help eliminate the memory leak issue.

Do you means that if the group has been initialized, then this function should not call initializeNewGroupsInternal again? But when we process a new window frame, we need to initialize the group again to clear the result of the latest window frame. So I call destroy in AggregateWindow

Also, does this problem only occurs when isFixedSize() false?

Maybe not. I find that NonNumericArbitrary does not override isFixedSize but we have to call destroy on it.

  virtual bool isFixedSize() const {
    return true;
  }
The initializeNewGroupsInternal should only be called when the condition is satisfied.

So this may be not correct.
Thanks for your reply.

jinchengchenghh · 2025-12-03T12:16:22Z

I mean destroy the allocator in initializeNewGroups not initializeNewGroupsInternal, this can help all the group initialization issue, but looks like community prefers to directly call the destroy function, so let us align with the current solution, find one and solve one.

jinchengchenghh · 2025-12-03T12:17:43Z

In the code and review, we may be carefully call the initializeNewGroups, decide if it should call destroy first

velox/exec/AggregateWindow.cpp

jinchengchenghh

Thanks for your fix!

jiangjiangtian · 2025-12-10T02:25:51Z

@mbasmanova @xiaoxmeng @Yuhta Could you please help review this PR?

mbasmanova · 2025-12-10T15:24:25Z

velox/exec/AggregateWindow.cpp

        // aggregate_ function object should be initialized.
        auto singleGroup = std::vector<vector_size_t>{0};
        aggregate_->clear();
+        aggregate_->destroy(folly::Range<char**>(&rawSingleGroupRow_, 1));


Do we still need to call 'clear' if we are calling 'destroy'?

Maybe we need. clear clears some states of the aggregate function and when we start processing a new window frame, we need to clear the states.

@jiangjiangtian @mbasmanova I too feel its confusing to call both these. Why does clearing an aggregate not imply destroying all the groups ?

Shall we add call to destroy in clear?

@mbasmanova @aditi-pandit how do you think? should we add call to destroy in clear?

@jiangjiangtian clear() is clearing the state, but doesn't free memory. Similar to std::vector::clear. destroy() frees up memory (and also clears the state). Similar to std::vector's destructor. I would imagine it is sufficient to call destroy(). Calling clear() before destroy() is redundant. Is this not the case?

@mbasmanova Now destroy won't clear the states of Aggregate functions. So should we call clear() in destroy()?

destroy won't clear the states

That's strange. Why would we need to explicitly clear the state if are releasing memory used by these. Perhaps, there are bugs in some implementations of destroy. What happens when you remove 'clear' call before 'destroy'?

What happens when you remove 'clear' call before 'destroy'?

Sorry, I have not tried it yet. But I find that 'clear' just sets the null count in Aggregate to 0 and the null count is just used to determine whether we need to actually extract bits from 'group' when we check if the 'group' is null or when we want to clear null bits of the 'group'.
After reading some code, I realize that we have to call clearNull in destroy. I think that is what we need to do. Before we destroy the accumulator, we need to check whether it is null. If it is, we need to subtract it from the null count in Aggregate to keep it aligned with the number of null groups allocated. Now we don't perform this check, so maybe that's the bug in destroy.
Based on the investigation above, I agree with you that the 'clear' call is confusing. So if we call clearNull in destroy, we can resolve the confusion. What do you think? Thanks! cc @mbasmanova

@jiangjiangtian Yes, I believe you are correct that destroy needs to clear the null flag.

mbasmanova

Wondering if it would be possible to reproduce this issue with a simple test and verify it is fixed?

mbasmanova · 2025-12-10T15:25:10Z

CC: @jiangjiangtian

velox/exec/tests/WindowTest.cpp

jiangjiangtian · 2026-03-06T06:30:50Z

@mbasmanova I iterate this PR based on #15680. Please take a look again, thanks!

mbasmanova

@jiangjiangtian Would you confirm that the test fails without the changes?

mbasmanova · 2026-03-06T16:41:07Z

/claude-review

github-actions · 2026-03-06T16:43:08Z

✅ Claude Code Review

Requested by @mbasmanova

Summary

This PR addresses a legitimate memory leak in the AggregateWindow operator by properly destroying previously created accumulators before reinitializing them. The fix is correct and necessary for memory safety.

Issues Found

🟢 Positive: Fix addresses real memory leak

The PR correctly identifies and fixes a memory leak where SingleValueAccumulator objects were being overwritten without proper destruction in window operations. The fix ensures destroy() is called before initializeNewGroups().

🟢 Positive: Proper API usage

The change from aggregate_->clear() to aggregate_->destroy() is the correct approach:

clear() only resets internal counters (numNulls_ etc.) but doesn't free accumulator memory
destroy() properly calls destroyInternal() which handles cleanup of non-numeric accumulators

🟢 Positive: Test demonstrates fix effectiveness

The test changes show the memory capacity was reduced from 256MB to 192MB, demonstrating that the fix actually reduces memory usage as expected.

🟢 Positive: Thread safety maintained

The changes don't introduce any concurrency issues. The rawSingleGroupRow_ is accessed consistently within the same thread context.

🟢 Positive: Consistent fix across code paths

Both incremental and non-incremental aggregation code paths in AggregateWindow.cpp were updated consistently (lines 153→destroy and 339→destroy).

Edge Cases Examined

Null handling: ✅ The added clearNull(group) in the destroy method ensures null state is properly reset
Multiple reinitializations: ✅ The fix works correctly when initializeNewGroupsInternal is called multiple times
Memory pressure scenarios: ✅ Test validates behavior under constrained memory
Exception safety: ✅ If initializeNewGroups throws after destroy, the state remains consistent

Performance Impact

The performance impact should be minimal since:

destroy() was already being called in other code paths
The operations are O(1) per group
Memory savings likely outweigh any marginal cost increase

Correctness Verification

I traced through the logic:

destroy() calls destroyInternal() which properly frees SingleValueAccumulator memory for non-numeric types
Resets the initialized flag (group[initializedByte_] &= ~initializedMask_)
Calls clearNull() to reset null state
initializeNewGroups() can then safely create new accumulators

The fix is correct and complete. The memory leak is properly addressed without introducing new issues.

ℹ️ About this response

This review was generated by Claude Code. It analyzed the PR diff and codebase to provide feedback.

Limitations:

Claude may miss context from files not in the diff
Large PRs may be truncated
Always apply human judgment to AI suggestions

Available commands:

/claude-review - Request a code review
/claude-query <question> - Ask a question about the PR or codebase

jiangjiangtian · 2026-03-09T09:49:33Z

@jiangjiangtian Would you confirm that the test fails without the changes?

@mbasmanova Yes! The test will fail if I revert this PR or #15680.

jiangjiangtian requested review from jinchengchenghh, rui-mo and zhli1142015 as code owners December 3, 2025 04:00

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 3, 2025

jiangjiangtian changed the title ~~fix: Destroy SingleValueAccumulator when there exists in FirstLastAggregateBase to fix memory leak~~ fix: Destroy previous accumulator when group has been initialized to fix memory leak Dec 3, 2025

jiangjiangtian changed the title ~~fix: Destroy previous accumulator when group has been initialized to fix memory leak~~ fix: Destroy previous created accumulator when group has been initialized to fix memory leak Dec 3, 2025

jiangjiangtian force-pushed the destroy_value branch from 88504b7 to f684fd0 Compare December 3, 2025 10:10

jiangjiangtian changed the title ~~fix: Destroy previous created accumulator when group has been initialized to fix memory leak~~ fix: Destroy the previously created accumulator for AggregateWindow to fix memory leak Dec 3, 2025

jiangjiangtian force-pushed the destroy_value branch from f684fd0 to 7ecefee Compare December 3, 2025 10:11

jinchengchenghh approved these changes Dec 3, 2025

View reviewed changes

jinchengchenghh reviewed Dec 3, 2025

View reviewed changes

velox/exec/AggregateWindow.cpp Show resolved Hide resolved

jinchengchenghh self-requested a review December 3, 2025 12:22

jinchengchenghh approved these changes Dec 5, 2025

View reviewed changes

mbasmanova requested a review from aditi-pandit December 10, 2025 15:23

mbasmanova reviewed Dec 10, 2025

View reviewed changes

jiangjiangtian force-pushed the destroy_value branch from f57a9b1 to c86c41e Compare December 11, 2025 12:22

jiangjiangtian commented Dec 11, 2025

View reviewed changes

velox/exec/tests/WindowTest.cpp Outdated Show resolved Hide resolved

jiangjiangtian force-pushed the destroy_value branch from c86c41e to bfff504 Compare December 12, 2025 08:04

Destroy the previously created accumulator for AggregateWindow

52a4015

jiangjiangtian force-pushed the destroy_value branch 2 times, most recently from cf703b3 to 56d83c9 Compare March 4, 2026 03:53

mbasmanova reviewed Mar 6, 2026

View reviewed changes

mbasmanova requested review from Yuhta, kagamiori and xiaoxmeng March 6, 2026 16:42

fix

e6e7d0a

jiangjiangtian force-pushed the destroy_value branch from 56d83c9 to e6e7d0a Compare March 9, 2026 09:50

Conversation

jiangjiangtian commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox ready!

Uh oh!

jinchengchenghh commented Dec 3, 2025

Uh oh!

jiangjiangtian commented Dec 3, 2025

Uh oh!

jinchengchenghh commented Dec 3, 2025

Uh oh!

jinchengchenghh commented Dec 3, 2025

Uh oh!

Uh oh!

jinchengchenghh left a comment

Choose a reason for hiding this comment

Uh oh!

jiangjiangtian commented Dec 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova commented Dec 10, 2025

Uh oh!

Uh oh!

jiangjiangtian commented Mar 6, 2026

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova commented Mar 6, 2026

Uh oh!

github-actions bot commented Mar 6, 2026

✅ Claude Code Review

Summary

Issues Found

🟢 Positive: Fix addresses real memory leak

🟢 Positive: Proper API usage

🟢 Positive: Test demonstrates fix effectiveness

🟢 Positive: Thread safety maintained

🟢 Positive: Consistent fix across code paths

Edge Cases Examined

Performance Impact

Correctness Verification

Uh oh!

jiangjiangtian commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jiangjiangtian commented Dec 3, 2025 •

edited

Loading

netlify bot commented Dec 3, 2025 •

edited

Loading

jiangjiangtian commented Mar 9, 2026 •

edited

Loading