abhishekrb19 commented on code in PR #19627:
URL: https://github.com/apache/druid/pull/19627#discussion_r3847887167
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/ByteBufferHashTable.java:
##########
@@ -442,19 +433,159 @@ public int getGrowthCount()
}
/**
- * To maintain an accurate tracking of the maximum bytes used per query,
this function is to be called immediately
- * whenever either of {@link #size} or {@link #bucketSizeWithHash} is
changed.
+ * Called whenever {@link #size} or {@link #bucketSizeWithHash} changes, to
track {@link #maxMergeBufferUsedBytes} and
+ * the {@link #maxSpillProximity} peak. Proximity is recorded while {@code
size < regrowthThreshold} (the transient hit
+ * at intermediate growth boundaries is skipped, since the table then
grows); at the terminal level, parking at the
+ * threshold is the spill point and pins 1.0. Trim-and-swap tables ({@link
#recordsFillProximity()} == false) skip
+ * proximity entirely — see {@link #findBucketWithAutoGrowth} for their only
spill signal.
*/
protected void updateMaxMergeBufferUsedBytes()
{
maxMergeBufferUsedBytes = Math.max(maxMergeBufferUsedBytes, (long) size *
bucketSizeWithHash);
+ if (!recordsFillProximity()) {
+ return;
+ }
+ final int denominator = getSpillRegrowthThreshold();
+ if (denominator <= 0) {
+ return;
+ }
+ if (size < regrowthThreshold) {
+ // size < regrowthThreshold <= terminal denominator keeps this below
1.0; the clamp is defensive.
+ final double ratio = Math.min(1.0, (double) size / denominator);
+ if (ratio > maxSpillProximity) {
+ maxSpillProximity = ratio;
+ }
+ } else if (isTerminalTableLevel()) {
+ // At the load-factor limit with no room to grow: parking here IS the
spill point.
+ maxSpillProximity = 1.0;
+ }
+ }
+
+ /**
+ * Denominator for {@link #maxSpillProximity}: the {@code regrowthThreshold}
at the terminal growth level, where
+ * {@link #findBucketWithAutoGrowth} can no longer allocate a bucket.
Computed once from fixed geometry and cached.
+ */
+ protected final int getSpillRegrowthThreshold()
+ {
+ if (spillRegrowthThreshold == 0) {
+ spillRegrowthThreshold = computeSpillRegrowthThreshold();
+ }
+ return spillRegrowthThreshold;
+ }
+
+ /**
+ * Replays the arena geometry to the terminal growth level — via the same
{@link #initialTableStart} /
+ * {@link #nextGrowthLevel} primitives {@link #reset()} and {@link
#adjustTableWhenFull()} use — and returns
+ * {@link #maxSizeForBuckets} of its bucket count. Allocates no buffers.
+ *
+ * <p>Valid only for the standard grow-by-doubling layout. Fixed-layout
variants (the alternating limit-pushdown
+ * table) must never reach this — guaranteed by {@link
#recordsFillProximity()} == false — and must override it if
+ * they ever need a spill denominator.
+ */
+ protected int computeSpillRegrowthThreshold()
Review Comment:
nit: `ccomputeSpillRegrowthThreshold()` and `getSpillRegrowthThreshold()`
methods can be `private` scoped
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java:
##########
@@ -224,9 +263,26 @@ public void mergeBufferAcquisitionTime(long delay)
mergeBufferAcquisitionTimeNs.addAndGet(delay);
}
- public void maxMergeBufferUsedBytes(long bytes)
+ /**
+ * Accumulates the peak merge-buffer usage of one grouper (slice). Despite
the previous "max" naming, this method
+ * sums across the slices a query holds; see {@link #mergeBufferUsedBytes}.
+ */
+ public void addMergeBufferUsedBytes(long bytes)
{
- maxMergeBufferUsedBytes.addAndGet(bytes);
+ mergeBufferUsedBytes.addAndGet(bytes);
+ }
+
+ /**
+ * Records one slice's peak fill ratio (1.0 iff it spilled), kept as a max
across the query's slices; see
+ * {@link #maxSpillProximity}. Clamped to [0, 1]; NaN is ignored so a
never-initialized grouper contributes nothing.
+ */
+ public void sliceUsage(double proximity)
+ {
+ if (Double.isNaN(proximity)) {
+ return;
+ }
+ final double clamped = proximity < 0.0 ? 0.0 : (proximity > 1.0 ? 1.0 :
proximity);
Review Comment:
nit:
```suggestion
final double clamped = proximity < 0.0 ? 0.0 : (Math.min(proximity,
1.0));
```
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java:
##########
@@ -224,9 +263,26 @@ public void mergeBufferAcquisitionTime(long delay)
mergeBufferAcquisitionTimeNs.addAndGet(delay);
}
- public void maxMergeBufferUsedBytes(long bytes)
+ /**
+ * Accumulates the peak merge-buffer usage of one grouper (slice). Despite
the previous "max" naming, this method
+ * sums across the slices a query holds; see {@link #mergeBufferUsedBytes}.
+ */
+ public void addMergeBufferUsedBytes(long bytes)
{
- maxMergeBufferUsedBytes.addAndGet(bytes);
+ mergeBufferUsedBytes.addAndGet(bytes);
+ }
+
+ /**
+ * Records one slice's peak fill ratio (1.0 iff it spilled), kept as a max
across the query's slices; see
+ * {@link #maxSpillProximity}. Clamped to [0, 1]; NaN is ignored so a
never-initialized grouper contributes nothing.
+ */
+ public void sliceUsage(double proximity)
Review Comment:
Naming: what do you think of a different method name like
`spillProximityUsage` so it doesn't seem like bytes usage per slice?
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/SpillingGrouper.java:
##########
@@ -249,7 +249,13 @@ public void reset()
public void close()
{
perQueryStats.dictionarySize(getDictionarySizeEstimate());
- perQueryStats.maxMergeBufferUsedBytes(getMaxMergeBufferUsedBytes());
+ final long sliceUsedBytes = getMaxMergeBufferUsedBytes();
+ perQueryStats.addMergeBufferUsedBytes(sliceUsedBytes);
+ if (grouper.isInitialized()) {
Review Comment:
I see `getMaxMergeBufferUsedBytes()` also calls `grouper.isInitialized()`,
the checks can perhaps be folded into one conditional block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
