Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/761#discussion_r103334294 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/xsort/managed/ExternalSortBatch.java --- @@ -948,50 +1027,50 @@ private void updateMemoryEstimates(long memoryDelta, RecordBatchSizer sizer) { // spill batches of either 64K records, or as many records as fit into the // amount of memory dedicated to each spill batch, whichever is less. - spillBatchRowCount = (int) Math.max(1, spillBatchSize / estimatedRowWidth); + spillBatchRowCount = (int) Math.max(1, preferredSpillBatchSize / estimatedRowWidth / 2); spillBatchRowCount = Math.min(spillBatchRowCount, Character.MAX_VALUE); + // Compute the actual spill batch size which may be larger or smaller + // than the preferred size depending on the row width. Double the estimated + // memory needs to allow for power-of-two rounding. + + targetSpillBatchSize = spillBatchRowCount * estimatedRowWidth * 2; + // Determine the number of records per batch per merge step. The goal is to // merge batches of either 64K records, or as many records as fit into the // amount of memory dedicated to each merge batch, whichever is less. - targetMergeBatchSize = preferredMergeBatchSize; - mergeBatchRowCount = (int) Math.max(1, targetMergeBatchSize / estimatedRowWidth); + mergeBatchRowCount = (int) Math.max(1, preferredMergeBatchSize / estimatedRowWidth / 2); mergeBatchRowCount = Math.min(mergeBatchRowCount, Character.MAX_VALUE); + targetMergeBatchSize = mergeBatchRowCount * estimatedRowWidth * 2; // Determine the minimum memory needed for spilling. Spilling is done just // before accepting a batch, so we must spill if we don't have room for a // (worst case) input batch. To spill, we need room for the output batch created // by merging the batches already in memory. Double this to allow for power-of-two // memory allocations. - spillPoint = estimatedInputBatchSize + 2 * spillBatchSize; + long spillPoint = estimatedInputBatchSize + 2 * targetSpillBatchSize; // The merge memory pool assumes we can spill all input batches. To make // progress, we must have at least two merge batches (same size as an output // batch) and one output batch. Again, double to allow for power-of-two // allocation and add one for a margin of error. - int minMergeBatches = 2 * 3 + 1; - long minMergeMemory = minMergeBatches * targetMergeBatchSize; + long minMergeMemory = Math.round((2 * targetSpillBatchSize + targetMergeBatchSize) * 1.05); // If we are in a low-memory condition, then we might not have room for the // default output batch size. In that case, pick a smaller size. - long minMemory = Math.max(spillPoint, minMergeMemory); - if (minMemory > memoryLimit) { - - // Figure out the minimum output batch size based on memory, but can't be - // any smaller than the defined minimum. - - targetMergeBatchSize = Math.max(MIN_MERGED_BATCH_SIZE, memoryLimit / minMergeBatches); + if (minMergeMemory > memoryLimit) { - // Regardless of anything else, the batch must hold at least one - // complete row. + // Figure out the minimum output batch size based on memory, + // must hold at least one complete row. - targetMergeBatchSize = Math.max(estimatedRowWidth, targetMergeBatchSize); - spillPoint = estimatedInputBatchSize + 2 * spillBatchSize; - minMergeMemory = minMergeBatches * targetMergeBatchSize; + long mergeAllowance = Math.round((memoryLimit - 2 * targetSpillBatchSize) * 0.95); + targetMergeBatchSize = Math.max(estimatedRowWidth, mergeAllowance / 2); + mergeBatchRowCount = (int) (targetMergeBatchSize / estimatedRowWidth / 2); --- End diff -- Good catch! Fixed.
--- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---