Skip to content

Avoid over-retaining memory in PartitioningExchanger #9327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sopel39 merged 1 commit into from
Sep 24, 2021
Merged

Avoid over-retaining memory in PartitioningExchanger #9327

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions core/trino-main/src/main/java/io/trino/operator/exchange/PartitioningExchanger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,20 +70,27 @@ private synchronized void partitionPage(Page page, Page partitionPage)

// build a page for each partition
for (int partition = 0; partition < partitionAssignments.length; partition++) {
IntArrayList positions = partitionAssignments[partition];
int partitionSize = positions.size();
IntArrayList positionsList = partitionAssignments[partition];
int partitionSize = positionsList.size();
if (partitionSize == 0) {
continue;
}
// clear the assigned positions list size for the next iteration to start empty. This
// only resets the size() to 0 which controls the index where subsequent calls to add()
// will store new values, but does not modify the positions array
int[] positions = positionsList.elements();
positionsList.clear();

Page pageSplit;
if (partitionSize == page.getPositionCount()) {
pageSplit = page; // entire page will be sent to this partition, no copies necessary
// entire page will be sent to this partition, just compact the page to avoid over-retaining
// memory and match the behavior of the sub-partitioned case
page.compact();
pageSplit = page;
}
else {
pageSplit = page.copyPositions(positions.elements(), 0, partitionSize);
pageSplit = page.copyPositions(positions, 0, partitionSize);
}
// clear the assigned positions list for the next iteration to start empty
Copy link
Copy Markdown
Member

@sopel39 sopel39 Sep 23, 2021

Choose a reason for hiding this comment

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

I think it's a bug. You use positions.elements() after you run positions.clear();

Copy link
Copy Markdown
Member Author

@pettyjamesm pettyjamesm Sep 23, 2021

Choose a reason for hiding this comment

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

Internally IntArrayList.clear() just sets IntArrayList#size = 0; but does not modify the int[] elements array. Reseting the size to 0 immediately after the if (size == 0) check simplifies the logic once the single partition case is refactored to return early (in the next PR) since otherwise you would have to ensure the clear() call happens in both branches.

Copy link
Copy Markdown
Member

@sopel39 sopel39 Sep 23, 2021

Choose a reason for hiding this comment

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

Internally IntArrayList.clear() just sets IntArrayList#size = 0; but does not modify the int[] elements array

I know, but it doesn't look good and you don't know if implementation of clear won't change in the future.

Copy link
Copy Markdown
Member Author

@pettyjamesm pettyjamesm Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Per the IntArrayList javadoc:

This class implements a lightweight, fast, open, optimized, reuse-oriented version of array-based lists. Instances of this class represent a list with an array that is enlarged as needed when new entries are created (by doubling its current length), but is never made smaller (even on a clear()). A family of trimming methods lets you control the size of the backing array; this is particularly useful if you reuse instances of this class.

I've made the following changes in case these are enough to satisfy you on the justification of calling clear() in advance of using the array:

  • Enhance the comment to mention this assumption
  • Store a local variable int[] elements = positions.elements() before calling clear()

If you still have some concerns I can drop that change out of this PR since it's not strictly required for this PR, and we can resolve how best to handle it as part of the second PR.

positions.clear();
memoryManager.updateMemoryUsage(pageSplit.getRetainedSizeInBytes());
buffers.get(partition).accept(new PageReference(pageSplit, 1, onPageReleased));
}
Expand Down